AGS 3.6.0.35 (Linux and OSX crash)

[eri0o]

I used a debugger in Linux, and you can see the first comparison to check the type from the bitmap fails, instead o 19778, bfType is 4,294,967,295 (2^32-1). Digging a bit deeper, the getc doesn't use the getc we put in the vtable, and instead it uses normal_getc from allegro's file.c. Not sure if this is happening in Windows too or why it works fine there.

[Crimson Wizard]

I used a debugger in Linux, and you can see the first comparison to check the type from the bitmap fails, instead o 19778, bfType is 4,294,967,295 (2^32-1). Digging a bit deeper, the getc doesn't use the getc we put in the vtable, and instead it uses normal_getc from allegro's file.c. Not sure if this is happening in Windows too or why it works fine there.

Yes, it uses normal vtable, because it goes through load_bitmap -> load_bmp which creates a default allegro's PACKFILE. On Windows it goes through the same functions, but the read result is correct.

There might be a mistake in how the PACKFILE's buffer is filled earlier (normal_refill_buffer), for instance, or some file operations. Maybe the compilation uses wrong library file functions because of mismatching flags or something; but I have not searched that far yet.

On the other hand, the solution may be to instead not use load_bitmap, but load_bmp_pf and load_pcx_pf, and pass a custom PACKFILE created from ags streams (similar to PackfileFromAsset). This should also make it work from Android package.

[Crimson Wizard]

@Crimson Wizard do you want to try the bitmap streamer road again? I think it was almost there except for some palette conversion stuff.

Oh well, I guess this could also work as an alternative. But only if it has complete necessary functionality, and there's also PCX support for older games, and a switch based on file extension (may be in a helper function).
Then all the bmp/pcx code could be removed from allegro sources, as well as all the "normal packfile" functions, since the only remaining PACKFILE users is video player and it uses our custom implementation anyway.

I still don't know if it's worth to do right now (i'd honestly rather try one of those image loading libs, like stb_image, if I remember the name correctly... but after 3.6.0 release).

[eri0o]

Apparently the palette stuff it was already fixed, so it's just the pcx loading! Edit: added pcx support!

I think the bmp stuff is better handled by us though because allegro4 had some particular things regarding palette and depth convertion that I think we will end up handling ourselves anyhow to keep things backwards compatible.

About libraries, there is SDL_image, it's more complex than stb_image though, but it would the sdl_rwops you already solved. I find stb_image more practical though - would not make tools also sdl dependant, not sure this is a desirable feature or not.

[eri0o]

Added the pcx stuff in the Bitmap Streamer PR.

I was reading the packfile stuff, and I wonder if maybe deleting all the "normal" packfile vtable code would fix, since it should always use the AGS packfile instead. This refactoring deletion is a lot of work as the allegro stuff is a bit intricate in the insides.

Edit: ah, actually it doesn't use our packfile at all, it uses allegro load_bitmap directly in Bitmap::LoadFromFile(const char *filename), so it's using allegro's own file method to open the file.

It looks like pack_fopen is simply loading with f.normal.buf_size == -1

Edit2: apparently this lseek is returning a ridiculously huge number, also I notice all things are aliased to some 64 variant in my linux machine. Apparently we have #define _FILE_OFFSET_BITS 64 in all but Windows builds currently. Maybe it's not useful there, I am not sure, I think allegro should be using off_t and other types that adjust with bitness, but it's using long and other types that do not.


@Dualnames if you want to test the version that uses the Bitmap Streamer, the PR is here and the the build on CI here

[Dualnames]

I will first thing tomorrow morning! Thank you so much for this, I was losing my mind for a second when i was investigating this whole issue!

[Crimson Wizard]

Edit: ah, actually it doesn't use our packfile at all, it uses allegro load_bitmap directly in Bitmap::LoadFromFile(const char *filename), so it's using allegro's own file method to open the file.

Erm, this is what I've mentioned twice in my posts above... it creates a default packfile with default implementations.

The lseek issue of course explains the problem, it uses wrong file offset flags instead of proper allegro flags (meaning allegro's own compilation config is broken).

In any case, I think we need to either replace with our custom packfile (in order to have full Android streams support), or with fully custom reading anyway.

[eri0o]

@Crimson Wizard it almost worked, the bitmap streamer, I am having trouble with 8bit images with palette in 32bit game, the test game is AGSKart, which I attached in the PR, it looks almost there but there's something wrong that I can't figure it out.

@Dualnames , I fixed a bunch more little things in my PR, please get the most recent binaries/code from the CI/PR to test.

[Dualnames]

Just tested on linux, the read from file seems fine, the savetofile not so much, it returns a segmentation fault. What I mean by that, is that when the game is reading the savegames doing the "CreateFromFile" for dynamic sprites it seems proper, but when it attempts to save a dynamic sprite to a file it returns a "Segmentation fault (core dumped)
" on Linux, happy to offer any further terminal logs etc. (this was tested with https://cirrus-ci.com/task/4875192413978624

[Crimson Wizard]

Just tested on linux, the read from file seems fine, the savetofile not so much, it returns a segmentation fault. What I mean by that, is that when the game is reading the savegames doing the "CreateFromFile" for dynamic sprites it seems proper, but when it attempts to save a dynamic sprite to a file it returns a "Segmentation fault (core dumped)
" on Linux, happy to offer any further terminal logs etc. (this was tested with https://cirrus-ci.com/task/4875192413978624

Please try the latest build: https://cirrus-ci.com/build/5168338358763520

I just tested this on 64-bit Ubuntu, and it seems to work (bmp file is saved correctly and reloaded back). Although I don't remember if there have been any significant changes since the version you mention above.

[eri0o]

@Dualnames can you make a small game that reproduces this segfault somehow? I tested a bunch of ideas and could not trigger this on Linux (or Windows, ...).

Or alternative send me the game that causes the issue in Discord and I will take a look when I am back home.

[Dualnames]

Sent you both a dm (eri0 on disc, cw in here)

[eri0o]

ok, so the problem it seems that DynamicSprite.SaveToFile is called with a path that includes a subdirectory before the file name "$SAVEGAMEDIR$/subdir/image.bmp", and this subdir is not being created for some reason. Looking at the code I am not sure how such directory is supposed to be created, I can't seem to figure out which code tells what are these subdirectories.

[Crimson Wizard]

ok, so the problem it seems that DynamicSprite.SaveToFile is called with a path that includes a subdirectory before the file name "$SAVEGAMEDIR$/subdir/image.bmp", and this subdir is not being created for some reason. Looking at the code I am not sure how such directory is supposed to be created, I can't seem to figure out which code tells what are these subdirectories.

It's the function ResolveWritePathAndCreateDirs; it uses ResolveScriptPath to find out which part of the path it is allowed to create and then calls Directory::CreateAllDirectories.

EDIT:
It would be nice to have the path with subfolders mentioned earlier, then I would not have to download a 1.5 gb game test...
As soon as I tested the path with subfolders, even on Windows, I immediately get a error with null pointer returning from the DynamicSprite.CreateFromFile in an old build, and a segfault within the new 3.6.0 build.

[eri0o]

This is not documented in the manual (or perhaps I did not notice it?), but I really did not test for this when making the stream stuff... It looks like the rp.Loc.SubDir.IsEmpty() is always true inside ResolveWritePathAndCreateDirs, so it returns true.

[Crimson Wizard]

This is not documented in the manual (or perhaps I did not notice it?), but I really did not test for this when making the stream stuff...

Maybe not... Any writing operation from script, including File.Open (with eFileWrite or FileAppend) and DynamicSprite.SaveToFile, precreate all the subdirectories in the path, that is all dirs following the location token.

This should be true since 3.5.0 (judging by the Changes.txt).

It looks like the rp.Loc.SubDir.IsEmpty() is always true inside ResolveWritePathAndCreateDirs, so it returns true.

Yes, there's some logical error in the ResolveScriptPath that makes it loose information about the subpath.

It is important to find out when the error was introduced, at least test 3.5.1 once again...

[eri0o]

3.5.1 did not have SubDir in FSLocation. It looks like it's something from around 3.6.0.6, but I could not yet figure it out when it was broken.

Edit: Found that replacing line https://github.com/adventuregamestudio/ags/blob/6aeb7d77ec15a61d208a012e4c855b4506155951/Engine/ac/file.cpp#L420

for

Code: c++

    parent_dir.SubDir = Path::GetDirectoryPath(child_path);
    String filename = Path::GetFilename(child_path);
    ResolvedPath test_rp = ResolvedPath(parent_dir, filename, alt_path);

doesn't fix it, because even though the directory IS created now, later the FileOpen that will create the stream will use a different path.


Edit2: create an issue to register this https://github.com/adventuregamestudio/ags/issues/1822 , added a small testgame in it

[Crimson Wizard]

There's a preliminary build with a fix:
https://cirrus-ci.com/task/5506153307176960

[Dualnames]

This seems resolved!