So, I'm trying Strangeland with 3.6, everything seems to work fine and perfectly, except on Linux and MacOS, it crashes whenever this function runs:
bigSave=DynamicSprite.CreateFromFile(String.Format("$SAVEGAMEDIR$/saveHD%d.bmp",getSlot));
Well, it doesn't crash there per se, it crashes after it, if you try to access bigSave or whichever dynamic sprite you assigned with CreateFromFile. So to summarize, only on LINUX AND ON MACOS, if you do "CreateFromFile" with a Dynamic Sprite and then try to access it, it goes kablooie.
(And yes the file in question exists. It returns a null pointer. I'm wondering if this an omission on my end and the pathing needs to be written differently depending on OS?)
First of all, to clarify, this "crash" is not a program crash, but engine reports a "null pointer" error in script because bigSave was not assigned? So, the issue is not really that it crashes (it errors logically), but that it cannot create a sprite from the file?
Where the file is located, and what is the save game dir? The engine log should print the game paths, including save dir, could you double check that?
How is the file supposed to appear in the save game dir in the first place?
So it doesn't crash, opening that file gives a null pointer? If that file exists there, and you not write to it just before, but reads later, does it still gives a null pointer?
My guess, disable disk cache on writing in Windows (https://learn.microsoft.com/en-us/troubleshoot/windows-server/backup-and-storage/turn-disk-write-caching-on-off), and see if you reproduce the behavior you are seeing in macOS and Linux.
From what I remember, in Windows the file writing is kinda of assynchronous, it goes to cache and then to the disk, so if you read before it's effectively on disk, Windows reads from the cache transparently. I thought that macOS and Linux engine would block the writing, so it would not fail to read because it would wait the write operation finish. So I don't know exactly why it's happening, but my first guess was this actual difference in OS behavior. In fact the blocking behavior was why in the past I suggested assynchronous save files/streams.
My suggestion would be simply handling the nullpointer case.
Just to check, have you just printed the slot number to check that you are actually reading the correct file?
If everything looks OK then you can probably work out what is going on by running the game through strace (https://strace.io/).
it is reading the correct file, for a second even the sprite is properly assigned to the UI button, and then it goes back to no graphic and then when the script reaccesses it it returns null. Which is absolutely weird.
Quote from: Dualnames on Sun 16/10/2022 10:46:17it is reading the correct file, for a second even the sprite is properly assigned to the UI button, and then it goes back to no graphic and then when the script reaccesses it it returns null. Which is absolutely weird.
If the bigSave variable is assigned properly first, then this means that the CreateFromFile call succeeded, at least the first call.
Do you have bigSave variable reassigned anywhere else in script? How often, under which conditions do you have this line called? In which context do you have this line in your script?
E.g. do you call it in a loop? or repeatedly over time perhaps?
So, here is the funny part.
bigSave=DynamicSprite.CreateFromFile(String.Format("$SAVEGAMEDIR$/saveHD%d.bmp",getSlot));
Wait(10);
bigSave.Resize(); //Resize is not what matters
This crashes. No matter what. The wait doesn't exist on my end. I am quite sure I don't do anything else after creating the sprite to cause this issue per se. This line gets called whenever you save the game. It runs once during that time.
Quote from: Dualnames on Sun 16/10/2022 23:48:46This crashes. No matter what.
Do I guess correctly that the crash is at bigSave.Resize() because of null pointer, and you get the "null pointer" script error on that line?
Have you tried testing for CreateFromFile return value right after it is called?
Testing if sprite was loaded correctly is a right approach anyway. CreateFromFile returning "null" is a valid behavior, this may happen in case the file does not exist, or could not be read for any reason (corrupted etc). Game script should not rely on this always succeeding.
Quote from: Dualnames on Sun 16/10/2022 23:48:46The wait doesn't exist on my end.
Could you please clarify what does that mean, what is "your end"?
You say the file exists but have you checked the result of Display("$SAVEGAMEDIR$")?
If this token isn't replaced by the proper path on non-Windows systems, the path is going to be invalid and the DynamicSprite pointer will point to null.
Luckily this is really easy to debug.
Quote from: Khris on Mon 17/10/2022 01:08:45You say the file exists but have you checked the result of Display("$SAVEGAMEDIR$")?
These tokens are not resolved to a path in Display command, only in File commands (and ListBox's FillDir).
There is an open feature suggestion for giving a function that returns these resolved values, but it has not been implemented yet.
You may find out what is the paths are by reading the engine logs, or running the engine or game executable with "--tell-filepath" command arg.
this page mentions how to do this: https://github.com/adventuregamestudio/ags/blob/master/OPTIONS.md
So prior to createfromfile, there is a check to check if the file exists.
So this is some sort of reading error. Investigating further.
(https://i.ibb.co/qdcNjWL/Untitled.png)
So the issue from what i can tell is besides being able to access the savegamesdir (I have cloud saves so it was trying to access the directories and files)
Is that it's unable to save into the $SAVEGAMEDIR$.
When there is a space on MAC, the Application Support part should be Application\ Support, otherwise it won't be able to read it.
So when the files are not there, it doesn't crash, but when they are it crashes being unable to read from the directory.
However this is the difference here, is that,
if (File.Exists("$SAVEGAMEDIR$/saveHD0.bmp"))
Reads properly and returns true. But
DynamicSprite.CreateFromFile("$SAVEGAMEDIR$/saveHD0.bmp");
Doesn't.
if (File.Exists(String.Format("$SAVEGAMEDIR$/saveHD%d.bmp",getSlot))&& doubleclickload)
{
if (bigSave!=null) bigSave.Delete();
bigSave=DynamicSprite.CreateFromFile(String.Format("$SAVEGAMEDIR$/saveHD%d.bmp",getSlot));
bigSave.Resize(gwidth, gheight);
}
I'm wondering if the Delete is the outlier, so gonna test this by removing the delete part!
EDIT: It's not.
Further investigation:
This is 100% replicatable, with a new project, same code.
Put this on room_load or room_after fade in.
Create a dynamic sprite variable naming it "spriteFile"
bool isnull=false;
function room_Load()
{
if (spriteFile==null && File.Exists(String.Format("$SAVEGAMEDIR$/saveHD%d.bmp",0)))
{
spriteFile=DynamicSprite.CreateFromFile(String.Format("$SAVEGAMEDIR$/saveHD%d.bmp",0));
bool getexists=File.Exists(String.Format("$SAVEGAMEDIR$/saveHD%d.bmp",0));
int a=0;
while (spriteFile==null && a<100)
{
spriteFile=DynamicSprite.CreateFromFile(String.Format("$SAVEGAMEDIR$/saveHD%d.bmp",0));
a+=1;
}
if (spriteFile==null)isnull=true;
else isnull=false;
AbortGame(String.Format("File exists: %d %d", getexists, isnull));
}
}
1) Run the game once. It should return "File exists: 0 1"
2) Create a bmp called saveHD0.bmp in your savegames directory of this project.
3) Start the game again it will return "File exists: 1 1" but only on LINUX and OSX. On Windows it will return "File Exists: 1 0"
Either, I'm absolutely stupid, or this shouldn't be the case.
So, it's a question of whether CreateFromFile is working properly on Linux and Mac; whether it a) works at all and whether it b) works with particular paths.
I guess the easier test could be to use File.Open.
Speaking of system differences, besides from being unix systems, Linux and Mac have case-sensitive filepaths, so that might also affect file reading.
Quote from: Dualnames on Mon 17/10/2022 05:59:52So the issue from what i can tell is besides being able to access the savegamesdir (I have cloud saves so it was trying to access the directories and files)
Is that it's unable to save into the $SAVEGAMEDIR$.
When there is a space on MAC, the Application Support part should be Application\ Support, otherwise it won't be able to read it.
So when the files are not there, it doesn't crash, but when they are it crashes being unable to read from the directory.
So, this is a problem with incorrect $SAVEGAMEDIR$ path on MAC?
When you say "crashes being unable to read from the directory" here are you still referring to the same "null pointer" error when accessing the dynamic sprite pointer, or is it another error?
Quote from: Crimson Wizard on Mon 17/10/2022 15:50:14Linux and Mac have case-sensitive filepaths
It is file-system dependent, but I believe the default one on macOS is case insensitive. i.e. if the problem was the case then macOS and Windows would likely be doing something similar and a Linux system would be doing something different.
I finally had time to test this properly, and I confirm that DynamicSprite.CreateFromFile does not work on Linux, regardless of the path.
File.Open does work for exactly same file path.
This means that either:
- DynamicSprite.CreateFromFile does or does not do something that File.Open does (to path or to file);
- The bitmap loading is broken on Linux/OSX.
The script I used for the test:
// room script file
function TestLoadImage(String filename)
{
if (File.Exists(filename)) {
Display(String.Format("%s detected", filename));
} else {
Display(String.Format("%s NOT detected", filename));
}
File *f = File.Open(filename, eFileRead);
if (f != null) {
Display("Opened %s", filename);
f.Close();
} else {
Display("Failed to open %s", filename);
}
DynamicSprite *dspr = DynamicSprite.CreateFromFile(filename);
if (dspr != null) {
Display(String.Format("Sprite was created from %s", filename));
dspr.Delete();
} else {
Display(String.Format("Failed to create sprite from %s", filename));
}
}
function room_AfterFadeIn()
{
TestLoadImage(String.Format("saveHD%d.bmp", 0));
TestLoadImage(String.Format("$SAVEGAMEDIR$/saveHD%d.bmp", 0));
}
First test tries to open file / create image in the game's directory, another in the save dir. Result is the same (assuming you have bmps in both).
For both paths the Windows results:
- saveHD0.bmp detected
- Opened saveHD0.bmp
- Sprite was created from saveHD0.bmp
The Linux results:
- saveHD0.bmp detected
- Opened saveHD0.bmp
- Failed to create sprite from saveHD0.bmp
After more in-depth debugging, this seem to be an issue with a PACKFILE reading again.
Here where it's done in the allegro4 library:
https://github.com/adventuregamestudio/ags/blob/fbb87c3370501d9acad9c7282c5e951032587c9e/libsrc/allegro/src/bmp.c#L594-L616
If I add a direct file reading using `fread`, then it reads the bitmap header perfectly.
But the allegro's PACKFILE functions read some garbage instead, so the function fails.
In this case the allegro is using its default PACKFILE reading operations. In other cases we provide our custom readers, and they work...
Either something got broken recently, or this is again a consequence of non-careful stripping of allegro.
PS. I think one quick solution could be to actually open our custom packfile for this too; except we will have to detect format from extension ourselves.
EDIT: hmm, to think of it, it should use a custom packfile with our streams anyway, because otherwise it won't be able to read image files stored in Android package.
PPS. But of course one thing that is still confusing is why would this work on Windows and not on Linux/OSX.
So, this might have something to do with 32/64-bit differences, or standard library differences (again, the types and sizes of variables).
@Crimson Wizard do you want to try the bitmap streamer (https://github.com/adventuregamestudio/ags/pull/1682) road again? I think it was almost there except for some palette conversion stuff.
Quote from: eri0o on Tue 18/10/2022 09:38:38@Crimson Wizard do you want to try the bitmap streamer (https://github.com/adventuregamestudio/ags/pull/1682) road again? I think it was almost there except for some palette conversion stuff.
No; not in 3.6.0 at least, and there was also PCX format support missing in that replacement, which might be necessary for backwards support.
Overall, I'd rather wish we used some image library that does multiple format reading and writing to save on code maintenance, and only make our own format reading as a last resort.
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.
Quote from: eri0o on Tue 18/10/2022 13:07:30I 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.
Quote from: eri0o on Tue 18/10/2022 09:38:38@Crimson Wizard do you want to try the bitmap streamer (https://github.com/adventuregamestudio/ags/pull/1682) 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).
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.
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 (https://github.com/adventuregamestudio/ags/blob/fbb87c3370501d9acad9c7282c5e951032587c9e/libsrc/allegro/src/file.c#L773) 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 (https://github.com/adventuregamestudio/ags/pull/1682) and the the build on CI here (https://cirrus-ci.com/task/4940279823204352)
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!
Quote from: eri0o on Tue 18/10/2022 19:01:26Edit: 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.
@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 (https://cirrus-ci.com/task/4875192413978624)/PR to test.
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
Quote from: Dualnames on Sat 22/10/2022 16:34:17Just 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.
@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.
Sent you both a dm (eri0 on disc, cw in here)
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.
Quote from: eri0o on Thu 27/10/2022 20:35:39ok, 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.
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.
Quote from: eri0o on Thu 27/10/2022 20:56:50This 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).
Quote from: eri0o on Thu 27/10/2022 20:56:50It 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...
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
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
There's a preliminary build with a fix:
https://cirrus-ci.com/task/5506153307176960
This seems resolved!