thobbsinteractive / magic-carpet-2-hd

Recode Binary code of game Magic Carpet2 to C/C++ language(remake MC2 for any platform)
GNU General Public License v3.0
30 stars 4 forks source link

Data file loading fails for lowercase named files. #119

Closed vanfanel closed 2 years ago

vanfanel commented 2 years ago

Hi there,

Got it to build on x86_64, but I see this with the latest GIT code:


Thread 1 "remc2" received signal SIGSEGV, Segmentation fault.
0x000055555570e487 in VGA_Set_Palette (Palettebuffer=0x0)
    at /home/manuel/src/magic-carpet-2-hd/remc2/portability/port_sdl_vga_mouse.cpp:457
457             memcpy(tempPalettebuffer, Palettebuffer, 768);
(gdb) bt
#0  0x000055555570e487 in VGA_Set_Palette(unsigned char*) (Palettebuffer=0x0)
    at /home/manuel/src/magic-carpet-2-hd/remc2/portability/port_sdl_vga_mouse.cpp:457
#1  0x000055555563ad9e in sub_41A90_VGA_Palette_install(TColor*) (bufferx=0x0)
    at /home/manuel/src/magic-carpet-2-hd/remc2/sub_main.cpp:38717
#2  0x00005555556abb96 in sub_84250_load_file_array(int) (psindex=1)
    at /home/manuel/src/magic-carpet-2-hd/remc2/sub_main.cpp:86305
#3  0x000055555566e1be in sub_5BF50_load_psxdata() () at /home/manuel/src/magic-carpet-2-hd/remc2/sub_main.cpp:60646
#4  0x000055555566d9bc in Initialize() () at /home/manuel/src/magic-carpet-2-hd/remc2/sub_main.cpp:60356
#5  0x0000555555662d0d in sub_main(int, char**, char**) (argc=1, argv=0x7fffffffe048)
    at /home/manuel/src/magic-carpet-2-hd/remc2/sub_main.cpp:55630
#6  0x0000555555563575 in main(int, char**) (argc=1, argv=0x7fffffffe048)
    at /home/manuel/src/magic-carpet-2-hd/remc2/remc2.cpp:27

This is after configuring simply with: cmake -DCMAKE_BUILD_TYPE=Debug ..

There was a 64bit specific config option but seems to be gone now, right?

GrimSqueaker commented 2 years ago

Hmm, what game data are you using? Do you have the GOG version or some other version? From the callstack it seems that you have entered a code path where there was a problem reading a file. Could you please comment out line 86305 in function sub_84250_load_file_array in file sub_main.cpp?

  sub_41A90_VGA_Palette_install((TColor*)*xadatapald0dat2.colorPalette_var28);//install Palette for text mode(show error)

Then it should print the file that caused the problem.

Improving the code that reads the original file assets is on our TODO list.

vanfanel commented 2 years ago

@GrimSqueaker After commenting out that line, this is what GDB gets me:


Thread 1 "remc2" received signal SIGSEGV, Segmentation fault.
__strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:120
120     ../sysdeps/x86_64/multiarch/../strlen.S: No such file or directory.
(gdb) bt
#0  __strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:120
#1  0x00007ffff7914f76 in __vfprintf_internal
    (s=s@entry=0x7fffffffc1c0, format=format@entry=0x555555761831 "ERROR: File %s.\n", ap=ap@entry=0x7fffffffc308, mode_flags=mode_flags@entry=0) at vfprintf-internal.c:1688
#2  0x00007ffff79201a4 in __vsprintf_internal
    (mode_flags=0, args=0x7fffffffc308, format=0x555555761831 "ERROR: File %s.\n", maxlen=18446744073709551615, string=0x7fffffffc320 "ERROR: File creating dirs\n") at iovsprintf.c:96
#3  __vsprintf
    (string=0x7fffffffc320 "ERROR: File creating dirs\n", format=0x555555761831 "ERROR: File %s.\n", args=0x7fffffffc308) at iovsprintf.c:105
#4  0x000055555570c4ba in myprintf(char const*, ...) (format=0x555555761831 "ERROR: File %s.\n")
    at /home/manuel/src/magic-carpet-2-hd/remc2/portability/port_outputs.cpp:22
#5  0x00005555556abb95 in sub_84250_load_file_array(int) (psindex=1)
    at /home/manuel/src/magic-carpet-2-hd/remc2/sub_main.cpp:86306
#6  0x000055555566e1be in sub_5BF50_load_psxdata() () at /home/manuel/src/magic-carpet-2-hd/remc2/sub_main.cpp:60646
#7  0x000055555566d9bc in Initialize() () at /home/manuel/src/magic-carpet-2-hd/remc2/sub_main.cpp:60356
#8  0x0000555555662d0d in sub_main(int, char**, char**) (argc=1, argv=0x7fffffffe058)
    at /home/manuel/src/magic-carpet-2-hd/remc2/sub_main.cpp:55630
#9  0x0000555555563575 in main(int, char**) (argc=1, argv=0x7fffffffe058)
    at /home/manuel/src/magic-carpet-2-hd/remc2/remc2.cpp:27

I am using the data files from my own copy of the game, bought back in the day here in Spain at budget price (not interested in GOG really, outside contemporary FNA games without DRM: mc2 is almost years old and I don't believe any of the original creators of the game see a dime from GOG sales. When I say creators I mean artists and coders, not investors which I couldn't care less for: sorry for the offtopic).

thobbsinteractive commented 2 years ago

Well I think it is owned by EA since they brought Bullfrog and ran it into the ground (it is a common EA pattern see Westwood etc..)

Anyway in the Rc2\Debug you need the following

remc2\Debug\CD_Files - This has the contents of the CD.
20/12/2020  19:53    <DIR>          DATA
20/12/2020  19:15           265,396 DOS4GW.EXE
20/12/2020  19:53    <DIR>          INTRO
20/12/2020  19:53    <DIR>          LANGUAGE
20/12/2020  19:53    <DIR>          LEVELS
20/12/2020  19:15         1,039,105 NETHERW.EXE
20/12/2020  19:15               183 NWSETUP.BAT
20/12/2020  19:15            19,126 README.TXT
20/12/2020  19:15           211,893 SETSOUND.EXE
20/12/2020  19:53    <DIR>          SOUND
remc2\Debug\NETHRW - This I believe was the original install data (post installation)
19/12/2020  23:03    <DIR>          .
19/12/2020  23:03    <DIR>          ..
19/12/2020  23:03    <DIR>          CDATA
19/12/2020  23:03    <DIR>          CLEVELS
18/05/2022  21:13                32 CONFIG.DAT
06/12/2020  23:30    <DIR>          LANGUAGE
25/04/2021  20:43    <DIR>          SAVE
06/12/2020  23:30    <DIR>          SHOTS
19/12/2020  23:03    <DIR>          SOUND
thobbsinteractive commented 2 years ago

I would love to upload said data but that would certainly violate copyright and get this whole thing shut down

vanfanel commented 2 years ago

@thobbsinteractive Oh, EA. Don't tell me more, don't tell me more. What a #?!#! of #@#@#?.

Thanks for being so patient with me. No need to upload the data, I will get this to work myself I hope.

Looking at config.ini (which comes with the sources and I have it next to the executable) I see this relevant part:

[main]
gameFolder = NETHERW ; Main game content folder (after install). Path is relative to .exe 
cdFolder = CD_Files ; Main game cd content folder (after install). Path is relative to .exe

So, next to the executable I also have:

manuel@vader:~/mc2$ ls CD_Files
data  dos4gw.exe  intro  language  levels  netherw.exe  nwsetup.bat  readme.txt  setsound.exe  sound

and


manuel@vader:~/mc2$ ls NETHERW
CDATA  CLEVELS  CONFIG.DAT  LANGUAGE  SAVE  SHOTS  SOUND

Both NETHERW and CD_Files need to be on the same directory as the executable, and so they are.

Also, had to copy over the font dir included with the sources, next to the executable.

Do you see something missing? I hope it's not the all-caps crap from Windows-land... :(

vanfanel commented 2 years ago

Ah, it was the all-caps thing in the end. It works after renaming all the CD_Files archives to uppercase recursively.

I think the loader should load the files even if they are lowercase, as is the case when extracting the files from an original MC2:NW CD.

thobbsinteractive commented 2 years ago

I agree. Can you re-title this issue? Then we can put in a fix.

vanfanel commented 2 years ago

Done! Is that OK? I am not an english expert at all.

thobbsinteractive commented 2 years ago

Interesting, Windows does not seem to be case sensitive.

thobbsinteractive commented 2 years ago

@vanfanel can you give this branch a test? https://github.com/thobbsinteractive/magic-carpet-2-hd/tree/fix/remove-case-sensitivity Please make sure you can finish the first level and that the scoreboard shows

GrimSqueaker commented 2 years ago

Ah, it was the all-caps thing in the end. It works after renaming all the CD_Files archives to uppercase recursively.

I think the loader should load the files even if they are lowercase, as is the case when extracting the files from an original MC2:NW CD.

You are right. And it is a problem I did run into too, when I started to contribute.

In a previous fork of the project I started to replace all file handling with C++ code. https://github.com/GrimSqueaker/remc2clean/blob/master/remc2/engine/file_handling.cpp I think I will try to port this here and support case-insensitive file handling.

@thobbsinteractive, @turican0: My idea was to have a class that loads all the game assets to RAM when the game is started. And then every code, where the game is currently loading directly from disk, will be replaced by getting the data from this file handling class. Is that ok or do you see any problem with regression testing or something else? If not, I would like to start working on that.

thobbsinteractive commented 2 years ago

My guess is that you would stream it all into ram, index it by path then whenever a fopen is called simple look in your index to see if it is already loaded and return the pointer? It is a good idea, my only concern is that is might effect Toms comparison testing somehow. But as long as it cleans up nicely I think it is a good idea. As stupid as it sounds if you include the scaled up graphics as well you could be looking at 300-400 MB of RAM. Not a huge amount, but if we ever want to run this on the Pi or Mobile system it is quite a lot. That said we can put configuration tags on it. Maybe make it an option in the config?

thobbsinteractive commented 2 years ago

@GrimSqueaker if you get time could you test https://github.com/thobbsinteractive/magic-carpet-2-hd/tree/fix/remove-case-sensitivity on linux? Also @turican0 do you know why they call sub_7AA70_load_and_decompres_dat_file(0, 0, 0, 0); with no parameters? I discovered it was an issue when trying to replace fopen with fopen_s. I imagine it is from the Bullfrog code. It causes a crash in the code if fopen_s is called without a path, yet fopen is okay.

GrimSqueaker commented 2 years ago

Hi @vanfanel

There was a 64bit specific config option but seems to be gone now, right?

The 64bit CMake flag was just needed as long as 64 bit was buildable and after that not playable. For this I made the default build on 64 bit systems a 32 bit build which needed 32 bit libraries that usually are not present on modern Linux systems anymore. With this I could first concentrate to make the 32 bit version work on Linux at all. However, this was always just a workaround until 64 bit was working well enough. Now I have removed that workaround, because @turican0 made 64 bit builds playable. On 32 bit systems (e.g. most Raspberry OS) however, the build will be a 32 bit build. You just cannot easily have a build anymore that is not the default bit-ness of your OS.

GrimSqueaker commented 2 years ago

@thobbsinteractive, I fear the fix is not as simple as this. A quick test without having looked at the details: If I for example rename CD_Files/DATA to CD_Files/data the game will quit with ERROR COPYING DATA FILES TO LOCAL DRIVE

Btw. @vanfanel , I would recommend to put the game data to ~/.local/share/remc2 and the config to ~/.config/remc2. Because then you do not have to copy the data all the time you clean up your build directory.

thobbsinteractive commented 2 years ago

I see, that is interesting. I guess I would need to check the fcaseopen code to see what it is doing or if it is even called. Thanks for testing it

turican0 commented 2 years ago

@GrimSqueaker - calling sub_7AA70_load_and_decompress_dat_file(0, 0, 0, 0); without parameters? I have no idea, maybe they originally had it to reset/clean it, but it looks to me like they forgot it in the code and now it does nothing. @thobbsinteractive - Toms comparison testing somehow. I'm comparing five maps - mapTerrainType_10B4E0, mapHeightmap_11B4E0, mapShading_12B4E0, mapAngle_13B4E0, mapEntityIndex_15B4E0, these shouldn't be affected. Occasionally framebuffer - but it's not a problem there either. The only more complicated structure is D41A0_0. There it would want a converter similar to convert_to_shadow_D41A0_BYTESTR_0(&D41A0_0, &shadow_D41A0_BYTESTR_0); - I use this on x64 because the structure can't be bit-compact due to pointers, and I convert it to bit-compact. The converter would also be useful as a guide to keep track of variable moves/renames.

thobbsinteractive commented 2 years ago

@thobbsinteractive, I fear the fix is not as simple as this. A quick test without having looked at the details: If I for example rename CD_Files/DATA to CD_Files/data the game will quit with ERROR COPYING DATA FILES TO LOCAL DRIVE

Nuts. That solution uses filesystem to search directories using a case insensitive string compare. It should work. To test it I need to run on Linux so I cannot debug it just yet. Time to dig out the Pi.

turican0 commented 2 years ago

Actually I don't expect anyone to play PI, it can be seen more as a sport for youtubers :) More platforms can improve the runtime on the existing ones, but it takes a lot of care not to break anything essential.

GrimSqueaker commented 2 years ago

Actually I don't expect anyone to play PI, it can be seen more as a sport for youtubers :) More platforms can improve the runtime on the existing ones, but it takes a lot of care not to break anything essential.

Probably not on a Raspberry Pi. But there are Chromebooks and all modern MacBooks. And as ARM is more power efficient I would not be surprised to see other notebook manufacturers to also have some ARM notebooks available. The Pi was just the only available ARM platform I have here.

Nuts. That solution uses filesystem to search directories using a case insensitive string compare. It should work. To test it I need to run on Linux so I cannot debug it just yet. Time to dig out the Pi.

I will give it a more thorough test and debug into it. It was just a really quick test yesterday.

thobbsinteractive commented 2 years ago

Thanks for taking a look @GrimSqueaker. As for the Pi, I have the Pi400, it is 64bit Quad Core Arm processor and 4GB of RAM. So it matches some older Phone/Mobile specs. I brought it for the built in keyboard lol. I just upgraded the OS to 64bit and I am trying to install a Git client.

GrimSqueaker commented 2 years ago

Ok, so for example renaming TMAPS0-0.DAT to tmaps0-0.dat seems to work and the code is called and behaves exactly as you want it to. However, if a directory is not in upper case it is not the file opening code that fails it is the code that checks if the directories are available. One example I gave above. Another example is renaming NETHERW/CDATA to NETHERW/cdata which fails with:

Original game not found in
 /home/sebi/.local/share/remc2/NETHERW folder
GrimSqueaker commented 2 years ago

Thanks for taking a look @GrimSqueaker. As for the Pi, I have the Pi400, it is 64bit Quad Core Arm processor and 4GB of RAM. So it matches some older Phone/Mobile specs. I brought it for the built in keyboard lol. I just upgraded the OS to 64bit and I am trying to install a Git client.

Cool. It is based on the Pi 4 - so the latest version. I tested on a Pi 3 which is not as powerful, but my Pi 4 is running home automation, a PaperlessNG docker container, and some other stuff. You can just go with VSCode which brings everything you need. Even a Git client.

thobbsinteractive commented 2 years ago

Wow. I had no idea how good VSCode was. I hate using the git command line. Thanks, that will save me some time.

thobbsinteractive commented 2 years ago

I will have another go at this, later this week. But I was having compilation issues with it finding (what I am guessing are the source code) for SDL2 etc...

thobbsinteractive commented 2 years ago

Now I have the pi building I can test my fix for this issue and debug why it is not working

thobbsinteractive commented 2 years ago

I do believe this is fixed