[Runtime Issue]: x86 savegames and demos not compatible with x64

ExperimentIV commented 5 months ago

Build Version

0e55dee0dec22b2895f14beb0c789bbbd489d5c3

Operating System Environment

[ ] Microsoft Windows (32-bit)
[ ] Microsoft Windows (64-bit)
[ ] Mac OS X
[X] Linux (specify distribution and version below)

CPU Environment

[ ] x86 (32-bit Intel/AMD)
[X] x86_64 (64-bit Intel/AMD)
[ ] ARM (32-bit)
[ ] ARM64 (64-bit; sometimes called AArch64)
[ ] Other (RISC V, PPC...)

Game Modes Affected

[ ] Single player
[ ] Anarchy
[ ] Hyper-Anarchy
[ ] Robo-Anarchy
[ ] Team Anarchy
[ ] Capture the Flag
[ ] Bounty
[ ] Entropy
[ ] Hoard
[ ] Monsterball
[ ] Cooperative

Game Environment

No response

Description

With a fresh CD install I was clicking different menus. I noticed in the View Demo menu I have a file that seems to ship with the game called Secret2.dem. When trying to run that file it will say that "boa not valid" and that the demo file is invalid. While not a show stopper I am not sure to the level of backward compatibility for such files are wanted.

On a side note the line that shows the full path will have the last character cut off.

Regression Status

I don't know.

Steps to Reproduce

Install game from CD. (Not sure if file is included on digital releases).
Go to View Demo menu.
Load Secret2.dem (I also don't have access to other demo files so I'm not sure if others will fail).

JeodC commented 5 months ago

Duplicate of #105

Jayman2000 commented 5 months ago

When trying to run that file it will say that "boa not valid" and that the demo file is invalid.

What happens after that? Does the demo playback normally?

ExperimentIV commented 5 months ago

When trying to run that file it will say that "boa not valid" and that the demo file is invalid.

What happens after that? Does the demo playback normally?

It will give an error "Bad demo file, unable to read" and then boot you back to the main menu.

Jayman2000 commented 5 months ago

It will give an error "Bad demo file, unable to read" and then boot you back to the main menu.

In that case, this issue is not a duplicate of #105. Here are some quotes from that issue:

On Windows, running SECRET2.DEM will display a "BOA Not Valid!" error message, but will then continue to demo playback.

This appears inconsequential to the game.

@JeodC I think that you may have misread this bug report. Considering the fact that it’s not a duplicate of #105, do you think that this issue should be reopened?

JeodC commented 5 months ago

I tested this with the Windows CD version and it loaded fine. I am unable to reproduce the issue on Windows. @donotsharepersonalinformation do you mind providing more info?

CPU
Which CD you used (presumably Linux Loki x86)?

ExperimentIV commented 5 months ago

  CPU op-mode(s):        32-bit, 64-bit
CPU(s):                  8
Vendor ID:               AuthenticAMD
  Model name:            AMD FX(tm)-8300 Eight-Core Processor

I installed from Window CD's through WINE. Well WINE didn't like swaping CD's, so I copied the contents from both into one directory locally and installed from there.

fac3l3ss79 commented 5 months ago

This also happens in my arm64 build on raspberry pi os bullseye 64 bit. I use a gog version of the game.

MaddTheSane commented 4 months ago

Same for me with macOS 14.4.1 on Apple Silicon. Data files from the macOS Steam release.

JeodC commented 4 months ago

@donotsharepersonalinformation Does this still occur? It's been a few commits since reported.

MaddTheSane commented 4 months ago

Part of the issue seems to be that sizeof(player) is 536, but the player structure stored in secret2.dem is 1280. Newly-created demos don't have this discrepancy. Maybe something changed in 1.5 that caused the player structure to become smaller?

JeodC commented 4 months ago

Part of the issue seems to be that sizeof(player) is 536, but the player structure stored in secret2.dem is 1280. Newly-created demos don't have this discrepancy. Maybe something changed in 1.5 that caused the player structure to become smaller?

The macOS steam release is 64bit, I don't believe we have fully implemented that yet. I may be wrong though. In @donotsharepersonalinformation's case, they may be using an unpatched version of the game (they didn't specify if they patched their CD assets to v1.4 first).

ExperimentIV commented 4 months ago

Part of the issue seems to be that sizeof(player) is 536, but the player structure stored in secret2.dem is 1280. Newly-created demos don't have this discrepancy. Maybe something changed in 1.5 that caused the player structure to become smaller?

The macOS steam release is 64bit, I don't believe we have fully implemented that yet. I may be wrong though. In @donotsharepersonalinformation's case, they may be using an unpatched version of the game (they didn't specify if they patched their CD assets to v1.4 first).

@JeodC I guess I was unclear as to what version I was using. I had installed base D3, then the Merc addon at which point I didn't look at the version. I later tried to apply the 1.4 patch and it would just error on me.

I just did an uninstall, re-installed base D3, applied the 1.4 patch then installed Merc after that. I can get the latest build off of main and try this again a little later.

Jayman2000 commented 4 months ago

@JeodC I guess I was unclear as to what version I was using. I had installed base D3, then the Merc addon at which point I didn't look at the version. I later tried to apply the 1.4 patch and it would just error on me.

You may need the Dual-Jewel fix. Look at the entry in descent3.com’s FAQ named “Q: I have the Dual-Jewel version of Descent 3, and when I try to install any of the patches, the installation terminates with an error message stating: 'Old File Not Found'. What do I do?”

I just did an uninstall, re-installed base D3, applied the 1.4 patch then installed Merc after that. I can get the latest build off of main and try this again a little later.

Unfortunately, that’s not the correct installation order. You’re supposed to install Descent 3, then install Descent 3: Mercenary and then install the v1.4 patch on top of that. That being said, you should be able to rescue your installation by installing the v1.4 patch again on top of your current installation. I haven’t actually tried doing that myself, but according to this DescentBB.net thread, it works.

ExperimentIV commented 4 months ago

Ok I started over again. Descent 3, then Mercenary, then 1.4 patch. I installed the latest build off of main and no longer get the "boa not valid" message, just the "bad demo file; unable to read" message now.

JeodC commented 4 months ago

Ok I started over again. Descent 3, then Mercenary, then 1.4 patch. I installed the latest build off of main and no longer get the "boa not valid" message, just the "bad demo file; unable to read" message now.

That error was commented out. Thanks for giving an updated status of the issue though.

GravisZro commented 4 months ago

So... I looked into the code behind this issue and... I'm surprised that demos ever worked in the first place.

There is one HUGE reason for why this demo doesn't work, struct player is directly read from and written to the save game.

It is not packed so a different compiler could add padding differently which results in different data structure.
It contains pointers! This means the value is only relevant if the compiler puts things in the same place!!
It contains instances of classes. This is the most bizarre part because classes aren't just data with separate functions, they have vtables.

IF loading old demos is ever going to work then it will require dissecting exactly how the old version structured player.

The demo function should be considered to be broken until a stable format is created because it basically requires the same compiler, platform, architecture and a very similar build.

JeodC commented 4 months ago

So... I looked into the code behind this issue and... I'm surprised that demos ever worked in the first place.

There is one HUGE reason for why this demo doesn't work, struct player is directly read from and written to the save game.

It is not packed so a different compiler could add padding differently which results in different data structure.

It contains pointers! This means the value is only relevant if the compiler puts things in the same place!!

It contains instances of classes. This is the most bizarre part because classes aren't just data with separate functions, they have vtables.

IF loading old demos is ever going to work then it will require dissecting exactly how the old version structured player.

The demo function should be considered to be broken until a stable format is created because it basically requires the same compiler, platform, architecture and a very similar build.

Interesting that the windows demos all work. I've tested the cd, gog, and steam with our artifacts. Perhaps we haven't changed so much yet on the windows side.

GravisZro commented 4 months ago

Interesting that the windows demos all work.

So after a closer look I have a better idea about this, Any demos that start with user_timeout_obj having a value (identifies a player's "timeout weapon") is likely to cause a problem because it's pointer to somewhere in Objects, a static array from object.cpp. However, besides that, some tricks were used to load/store the other data.

guided_obj (a pointer to Objects) is correctly restored by using the actual pointer as a boolean value as to whether the handle should be stored/loaded.
the inventory class contents are saved elsewhere (because they are pointers to dynamically allocated objects,) so whatever space they take up in the demo file is just a garbage values.

With a map of the offsets to each member of the player structure it would be possible to correctly map the save game data. However, the final component is to find out the address of Objects from one of the original 32-bit builds and use it to calculate which object to point to. The actual demo file read/write data appears to be done in a sane manner, so fixing this would restore both the save game and demo ability.

Ultimately, the "game save" system should be modified to function in a more sane manner that is endian agnostic but it looks like old data isn't lost forever.

GravisZro commented 4 months ago

@JeodC If you compile this code using the same MSVC that built the version of D3 that can load the demo file and paste the execution results then I should be able to map old save game data.

JeodC commented 4 months ago

@JeodC If you compile this code using the same MSVC that built the version of D3 that can load the demo file and paste the execution results then I should be able to map old save game data.

Windows can load all the demo files, but I don't believe I attempted the Linux steam demo. I doubt the game will compile in VC6 anymore.

JeodC commented 4 months ago

@JeodC If you compile this code using the same MSVC that built the version of D3 that can load the demo file and paste the execution results then I should be able to map old save game data.

With the advent of x64 windows builds I am able to finally reproduce the issue. Unfortunately, simply copy-pasting your gist caused the build to fail. Can you provide some more detailed instruction and show what the code does?

GravisZro commented 4 months ago

Unfortunately, simply copy-pasting your gist caused the build to fail. Can you provide some more detailed instruction

This not part of D3 project at all, it is a standalone program that should be compiled in isolation using the same MSVC compiler and target settings as the Windows x86 D3 build . To do this you have need the MSVC compiler was used to build your windows x86 D3 build that is capable of successfully playing back the demo file. You can either create a MSVC project with the same settings as D3 or simply copy and modify one of the commands used while compiling D3 for the target. If you copy one of the compilation commands, you will need replace the name of the source file to compile, and add an executable output option. Either way the resulting executable

and show what the code does?

The code will print a list of member offsets for the player struct. I've copied all the structs needed into the source.

I've updated the gist so that it will alert you if the build settings are incorrect.

Here's the build and output on my LInux x86_64 machine.

$ g++ savegameinfo.cpp -o savegameinfo
$ ./savegameinfo 
Player struct is 536 bytes.
The incorrect compiler settings have been used.
You should be building with MSVC for Windows x86
Structure 'player' should be 1280 bytes.

start_index = 0
start_pos = 4
start_roomnum = 16
start_orient = 20
startpos_flags = 56
ship_index = 60
callsign = 64
flags = 84
score = 88
damage_magnitude = 92
edrain_magnitude = 96
invul_magnitude = 100
energy = 104
lives = 108
level = 109
starting_level = 110
keys = 111
killer_objnum = 112
invulnerable_time = 116
last_hit_wall_sound_time = 120
last_homing_warning_sound_time = 124
last_thrust_time = 128
last_afterburner_time = 132
objnum = 136
team = 138
current_auto_waypoint_room = 140
time_level = 144
time_total = 148
num_hits_level = 152
num_discharges_level = 156
num_kills_level = 160
friendly_kills_level = 162
num_kills_total = 164
weapon_flags = 168
weapon_ammo = 172
weapon = 216
laser_level = 240
light_dist = 244
r = 248
g = 252
b = 256
ballspeed = 260
num_balls = 264
ball_r = 268
ball_g = 280
ball_b = 292
oldroom = 304
inventory = 312
counter_measures = 336
last_fire_weapon_time = 360
afterburner_mag = 364
thrust_mag = 368
afterburner_sound_handle = 372
afterburn_time_left = 376
thruster_sound_handle = 380
thruster_sound_state = 384
small_left_obj = 388
small_right_obj = 392
small_dll_obj = 396
multiplayer_flags = 400
last_multiplayer_flags = 401
last_guided_time = 404
tracker_id = 408
kills = 420
deaths = 424
suicides = 428
rank = 432
lateral_thrust = 436
rotational_thrust = 440
time_in_game = 444
guided_obj = 448
user_timeout_obj = 456
zoom_distance = 464
movement_scalar = 468
damage_scalar = 472
armor_scalar = 476
turn_scalar = 480
weapon_recharge_scalar = 484
weapon_speed_scalar = 488
piggy_objnum = 492
piggy_sig = 496
custom_texture_handle = 500
ship_permissions = 504
invul_vector = 508
controller_bitflags = 520
num_markers = 524
num_deaths_level = 526
num_deaths_total = 528

jcoby commented 4 months ago

i tried parsing the demo file in ImHex but it's nearly hopeless. you have to load the level during parsing of the file (loadstate.cpp:592; num_portals comes from the level data) to finish parsing the demo file and the player data is at the very end of the data. it's a wonder the demo gets as far as it does.

One thing to note is the dem file is also a version 1 file but the current demo parser is on version 2 (#define GAMESAVE_VERSION 2):

jcoby commented 4 months ago

Diving a bit deeper into what's actually being loaded, the player's callsign is " B\0\xc0ZF\0\0\0\0\0\0\x80?" or 204200c05a46000000000000803f in hex. This hex sequence only appears once in the dem file and it's in the middle of a whole lot of nothing. So I think we have a desync in data somewhere earlier.

GravisZro commented 4 months ago

It's notably different, even when using the same compiler settings as the source, so I'm not sure 1280 is the correct expected size for the player struct.

You are probably right which means the demo loading likely already broken by the time to gets to loading the player struct. I'll look into it.

jcoby commented 4 months ago

It's notably different, even when using the same compiler settings as the source, so I'm not sure 1280 is the correct expected size for the player struct.

You are probably right which means the demo loading likely already broken by the time to gets to loading the player struct. I'll look into it.

1280 is almost certainly the wrong size (see my comments above). It's just random data in the file.

I've verified the loader is ok until something in LGSObjects() is causing the desync in readback. There are several places sizeof is being used that could cause issues; LGSObjWB() et al.

I've been tracing through the code and verifying the format in ImHex and have made it to loadstate.cpp:880 successfully so I believe the desync is happening after that. I suspect it's in the object loading loop around line 950.

GravisZro commented 4 months ago

@jcoby I've looked over a bit of the code and noticed there are "canary values" that would trip if the loading was off, like this:

    gs_ReadInt(fp, sig);

    ASSERT(sig == 0xBADB0B);

This means it's at least making it to the last canary before failing.

GravisZro commented 4 months ago

Just FYI, I finally got a cross-compilation and execution environment and was able to make a build that would never go into graphics mode (I'm using non-graphic qemu) but would still load the save. My discovery is that ai_frame is the wrong size.

i386: sizeof(ai_frame): 3364 x86_64: sizeof(ai_frame): 3488

GravisZro commented 4 months ago

So, with a bit of dedication I was able to collect the names and member names of almost all the structs in D3 as JSON data (because I kept finding more structs that were in the format), write a script to generate a struct info dumper (and a couple classes), which of course leads to the final JSON data presently has information for both x86 and x86_64. The information will tell you the size of every struct, offset of every struct member, it's size (or array element size), if it (or array element) can be byteswapped, and number of array elements if it's an array. The plan is to get a PPC build which is why the byteswap info is needed.

My plan is to use the struct naming information to generate iostream functions to make a universal savegame/demo format which would be fully packed and not include pointers (WTF, mate?). Meanwhile the array structure/member offset data will be used merely to generate istream functions to load to load any of the cataloged structs. It should be possible to detect endianness using the demo canary values. I haven't looked at the save format so it may just be trying LE and if values are out of range try BE.

winterheart commented 4 months ago

ai_frame, object.rtype and player structs is differs on x64 by size. That all structs so far I've discovered that involved to save/load process.

We can use IOOps.h from HogMaker for iostreams serialization/deserialization. Save / Load functions don't use so much engine logic, they just saves / dumps internal state of game. We can rewrite these functions alone. There also demo save/load functions too, and they uses mostly same code.

GravisZro commented 4 months ago

ai_frame, object.rtype and player structs is differs on x64 by size.

That may be the case for this compiler with these setting but different compilation settings result in different struct/class sizes. There are many non-deterministic structs that have padding added and are being blindly copied. I'm not interested in merely putting a band-aid on the code to solve the problems right now when doesn't solve the root problem of non-deterministic data sizes. Furthermore, it does nothing at all to address endianness.

We can use IOOps.h from HogMaker for iostreams serialization/deserialization.

Frankly, std::ios architecture is a bit bloated for the simplicity of this task. Part of the issue is there is a large amount of function duplication because while they implement their own buffering for files, POSIX stream functions provide that exact capability. But more importantly, you cannot overload the stream operations for integer and floating point types which means any byteswapping must be done manually for every recorded struct.

I've previously developed a compact and robust set of data streaming classes loosely based on std::ifstream/std::ofstream that are suited for a job just like this.

winterheart commented 4 months ago

IOOps.h already has little/big-endian awareness and provides capabilities to extend any serialization / deserialization operations.

GravisZro commented 4 months ago

I've seen it and you aren't using a stream operator because you cannot overload the stream operators for integer and floating point types. I do appreciate the effort though.

winterheart commented 4 months ago

Yes, you cannot overload << and >>, this is why bin_read() and bin_write() on primitives was written. I don't see any issues with that.

GravisZro commented 4 months ago

details about struct types and how the x86 and x86_64 compare:

packed and sizes match:
  PercentageRange
  ai_mem
  ase
  axis_billboard_info
  blast_info_s
  death_info
  debris_info_s
  dying_info_s
  dynamic_face
  fusion_effect
  g_attach
  g_floats
  g_static_path
  g_steer
  g_wander_extra
  gi_fire
  ground_information
  guninfo
  line_info_s
  link_tile
  matrix
  node
  object_link_info
  old_vis_attach_info
  path_information
  player_weapon
  point_info
  portal
  powerup_info_s
  powerup_timer
  scanline
  soundsource_info_s
  state_limited_element
  static_proc_element
  terrain_normals
  terrain_render_info
  terrain_segment
  vector
  vis_attach_info
  weapon_hit_info

packed but sizes don't match:
  bn_node
  bsptree
  hotspot
  tGameCinematic
  tOSIRISScript

not packed but sizes match:
  ai_dynamic_path
  ai_move_path
  ai_path_info
  ain_hear
  ain_see
  ambient_life
  bn_edge
  bspplane
  cntrldata
  custom_anim
  door
  dynamic_cell
  dynamic_wb_info
  effect_info_s
  fireball
  goal_enabler
  laser_info_s
  level_info
  light_info
  lightmap_info
  lodoff
  multi_orientation
  netgame_info
  network_game
  notify
  old_vis_effect
  otype_wb_info
  physics_info
  player_fire_packet
  player_pos_suppress
  poly_wb_info
  postrender_struct
  powerup
  powerup_respawn
  roomUVL
  room_changes
  sHUDResources
  shard_info_s
  ship
  shockwave_info
  specular_instance
  spewinfo
  splinter_info_s
  tMissionData
  tMissionInfo
  t_ai_info
  team
  terrain_mine_list
  terrain_sky
  terrain_tex_segment
  vis_effect
  vmt_descent3_struct
  weapon
  weather

not packed with different sizes:
  Inventory
  ai_frame
  asp
  bn_list
  bspnode
  bsppolygon
  dynamic_lightmap
  face
  fog_portal_data
  game_event
  game_path
  goal
  goal_info
  hotspotmap_t
  inven_item
  matcen
  multi_turret
  object
  object_info
  pilot
  player
  polyobj_info
  proc_struct
  room
  special_face
  tCannedCinematicInfo
  tHUDItem
  tInvenInfo
  tInvenList
  tLevelNode
  tMission
  tOSIRISScriptNode
  tOSIRISTriggerScript
  texture
  trigger
  vclip
  window_box
  windowmap_t

Lgt2x commented 1 month ago

As mentioned in #506 , this won't land in 1.5. This would require a lot more work to get working, inspecting structs one by one...

DescentDevelopers / Descent3