Skip to content

Merge upstream's save container and object model into the port - #3

Merged
msnow345 merged 19 commits into
mainfrom
merge-upstream-0.2
Sep 10, 2026
Merged

Merge upstream's save container and object model into the port#3
msnow345 merged 19 commits into
mainfrom
merge-upstream-0.2

Conversation

@msnow345

Copy link
Copy Markdown
Owner

Merges the 17 upstream commits from OpenTS-Developers/OpenTS main (0281b88..945e858) into the macOS/iOS port. 49 conflicts, 39 of them under code/.

What the conflicts actually were

Upstream's 945e858 (OpenTS-Developers#140, replace the compound-file save container and remove COM from the object model) is the same work this fork already carried as 93613db and f43aae0, developed independently. In most conflicted files the two sides were 90% identical — code/savefile.cpp differed by 387 lines out of 571, code/drive.cpp by 13 out of ~250 — so picking a side blindly would have quietly reverted one fork's fixes or dropped the other's.

Default resolution: take upstream's shape, then re-apply only what this port genuinely needs. Divergence in code/ is now three lines.

Taken from upstream, replacing ours

Area What upstream's version does that ours did not
code/savefile.cpp/.h std::uint32_t throughout; CRC::Memory instead of a private CRC-32 table; writes the header, table and payload without copying them into one image first; Win32 file handles instead of std::FILE *
code/savestream.h BoundScope (a member reading past its own record is refused instead of spending the next record's bytes), Reserve (a count the process cannot hold fails the load instead of throwing), and a char[N] serializer
code/saveload.cpp/.h Load_Vector<T> and Load_Object_As<T>, so a record naming a class that does not belong in a heap fails the load; ownership through std::unique_ptr; the listing is read from the same pass as the content; std::bad_alloc and std::length_error are caught
code/classfactory.* std::unique_ptr creators
code/ini.cpp A real ClassID parser instead of sscanf over a brace-stripped copy, and a doc comment moved onto the function it describes
code/mouse.cpp Zone and subzone ids widened to int; ZoneAdjacency and the subzone connection tables moved to std containers; a second-pass count mismatch now fails the save instead of writing an unreadable map
code/vein.cpp PriorityQueueClass backed by a vector; no GrowthNodes array
code/revent.cpp Fixes a bug of ours. ~RadarEventClass already calls RadarEvents.Delete(this), so our extra Delete_Index(i) was removing a second, unrelated event on every iteration. Upstream also bounds the count before allocating
code/unit.cpp faceto = Direction(passenger).As_Dir256() replaces a self-initialised DirType direction = direction.Direction(...) (OpenTS-Developers#130)
code/droppod.cpp A pod that carries nothing stays the object's locomotor instead of handing itself away
code/swizzle.cpp Abandon no longer writes through the registered pointer slots; they were never filled
code/foot.cpp The route staging array is PATH_LENGTH_MAX + 1 rather than a bare 200*10
IPiggyback::Begin_Piggyback Takes std::unique_ptr<ILocomotion> &. This is a real fix. By value, a refused locomotor was destroyed and the caller's object left without one; by reference a refusal leaves it with the caller. Applied across drive, walk, droppod, building, infantry, unit, foot
tests/save, docs/SAVE-FORMAT.md, manual/ Upstream's, matching the code above
code/blowfish.h Unguarded #include "win.h", which removes an #ifdef _WIN32 from code/

Kept from this fork

Three lines, all of them port seams that upstream has no reason to carry:

  • code/taction.cppHost_Sleep(1000) and #include "hostclock.h" instead of Sleep(1000). Host_Sleep is the port's portable clock seam, used in eight files; the include was also moved below always.h, where docs/STYLE.md puts it.
  • code/techtype.cpp#include <new> rather than upstream's <new.h>, which is MSVC-only.
  • code/mouse.cppMouseClass::Get_Mouse_Frame_Rate, which the iOS mode icon reads.

Moved out of code/ and into platform/

Upstream's savefile.cpp and saveload.cpp call Win32 entry points the shim did not have. Rather than keep our portable-but-divergent versions, the missing entry points were added to platform/win32compat, which is where a platform difference belongs and which leaves code/ byte-identical to upstream:

  • GetFileSize, FlushFileBuffers, MoveFileExA (kernel.cpp), plus INVALID_FILE_SIZE and MOVEFILE_REPLACE_EXISTING. A HANDLE is already a std::FILE * here, so these are a dozen lines each.
  • GetSystemTimeAsFileTime, which lets code/saveload.cpp drop its own #ifdef _WIN32 epoch helper.

The save checksum is unchanged by the switch to CRC::Memory: its table is the standard reflected CRC-32 table, crc ^= 0xFFFFFFFF is ~seed, and (crc >> 8) & 0x00FFFFFF is crc >> 8 at 32 bits. Bit-for-bit the same value our private table produced.

The assertion guards, and one that fired

All 42 of upstream's #149 guards and every one of this fork's 79 are present; the tree now carries 91. Every number the two sides both assert agrees — AUD 12/8, CompHeaderType 8, IsoTileRecord 52, IsoTileSet 20, MIX FileHeader 6, PCX RGB 3 and PCX_HEADER 128, RGBStruct 3, ShapeRecord 24, ShapeSet 8, VQASN2J 12, VQAHeader 42. A second commit drops the fork's size guards that upstream's now duplicate exactly, keeping every offset guard, which upstream does not carry. The only expression removed that has no upstream twin in the same scope is sizeof(data) == 12, whose twin is sizeof(SNJ2Struct) == 12 three lines above it.

One upstream guard fired, and it found a real port defect. static_assert(sizeof(MSBitmap) == 58) in code/srfcache.cpp failed at 86 bytes. platform/win32compat has typedef long LONG, which is 8 bytes on LP64, so its BITMAPINFOHEADER was 64 bytes rather than 40 and every field after biSize sat at the wrong offset. SurfaceCacheClass::CacheBMP casts a raw .BMP file image straight onto that struct, so it would have read garbage — it has no callers today only because the port retired the owner-draw dialog system, so the bug was latent. Fixed in the shim: tagBITMAPINFOHEADER names a new LONG32 for the four fields the format fixes at four bytes, with static_assert(sizeof(BITMAPINFOHEADER) == 40) beside it. dsurface.cpp, wincursor.cpp and gdi.cpp use the same struct in-process and are consistent with the narrower one.

Things upstream would probably rather I had not done

  • OPENTS_ARCH falls back to CMAKE_SYSTEM_PROCESSOR. Upstream's d1d7c06 reads CMAKE_CXX_COMPILER_ARCHITECTURE_ID, which only MSVC sets, so every native and cross build would stamp unknown. One extra if.
  • enable_language(RC) stays inside if(WIN32). Upstream unguarded it; no other toolchain here has a resource compiler.
  • code/taction.cpp, code/techtype.cpp, code/mouse.cpp carry the three port lines above.
  • Three refusal assertions were dropped from tests/save/savetest.cpp by taking upstream's file: a block whose match reaches before the start, a block with no end marker, and a block that expands far past its length. They were ours. I did not re-add them, because the harness is MSVC-only and I could not run it to check whether lzo1x_decompress_safe still rejects those inputs the way our version assumed.

Save compatibility

Saves written before this merge will not load. The container is unchanged — same signature, FORMAT_VERSION still 1, same header layout, same CRC — but the game-state stream moved:

  1. Map zone and subzone ids widened from unsigned short to int, and the subzone connection tables changed shape.
  2. A char[N] member now travels as a length and its text; before, upstream's generic T (&)[N] overload wrote all N bytes raw.
  3. The listing no longer writes PIDSI_PLAYER_NAME1 or PIDSI_PLAYER_NAME2.

The packed version stamp is unchanged (0.2.0 both sides), so nothing refuses an old save on version grounds. It will instead fail on upstream's per-record length check — a clean, logged load failure with the class name and offset, not a silent misread. Only unreleased local saves are affected. Bumping SaveFileClass::FORMAT_VERSION would give a cleaner refusal but would diverge from upstream's numbering, which is a product decision.

One thing gets better: upstream's docs/SAVE-FORMAT.md now states that the swizzle identity and every pointer member travel as four bytes regardless of the build's pointer width, so an arm64 save and a Win32 save are no longer separated by that.

Verification

Both targets build clean from the merged tree:

$ cmake --build build/native --target OpenTS
[291/292] Linking CXX executable bin/Game

$ cmake --preset ios-device && cmake --build --preset ios-device
[291/292] Linking CXX executable bin/OpenTS.app/OpenTS
build/ios-device/bin/OpenTS.app/OpenTS: Mach-O 64-bit executable arm64

(Only the OpenTS target: the C++ harnesses use MSVC-only flags and have never built on macOS.)

Checked by inspection: grep -rn OPENTS_IOS code/ is still empty; all ten Win_* / Video_* / UI_Mode_Icon_* port seams are still wired to the same call sites; code/netdlg2.cpp is byte-identical to its pre-merge version, so all four Lobby_Seat_Is_Valid sites survive; no submodule pointer needed resolving; the class registration table is identical to upstream's, all 67 entries.

Not verified

Nothing was run. No build was launched, no game was played. At the user's request all runtime verification was dropped from this task; the list below is what that leaves open.

  • The game was never started, on macOS or iOS. Main menu, skirmish, campaign, movie playback and clean shutdown are all unverified.
  • No save was written or loaded. The save container, the object model and the swizzle path are the largest part of this merge and none of it has been exercised.
  • Nothing was deployed to a device. The bundle links; it has not run.
  • No automated test was executed. tests/save, tests/priorityqueue and the other 36 harnesses are MSVC-only and do not build here.
  • The supported Win32 MSVC build was not compiled. CI on this PR is the first check of it.
  • manual/tools/manage.py check and commands.py could not run: PyYAML is not installed here, and I did not install into the user's environment. manual/ still has no entry for the port's [Video] MaxFrameRate, which predates this merge.
  • Multiplayer was not tested, so the lobby validation is preserved by inspection only.

What to check when playtesting, in order

Ranked by how likely a resolution here is to have broken it, not by how easy it is to test.

  1. Save a game, quit the process, relaunch, load it. This exercises the entire replaced container and object model at once, plus the three new platform/ file entry points (MoveFileExA is what commits the .tmp into place — if it is wrong, the save silently never appears). Do this on macOS first. Then confirm the expected failure: a save from before this merge should refuse to load and put a record-length line in Debug/DEBUG_*.LOG, not crash and not load wrong.
  2. Load a save on a large map, then scroll the whole map and order units across it. Zone and subzone ids widened from unsigned short to int in the map's save and load, and the connection tables changed container. A wrong read here shows up as units refusing to path across the map, or pathing into water.
  3. Build a unit from a war factory and drive it out; then order infantry to move. Begin_Piggyback changed signature everywhere, and the war-factory exit in building.cpp and the two infantry sites in infantry.cpp are the paths that hand a locomotor over. A wrong resolution here shows as a unit frozen in the factory doorway or an infantryman that will not walk.
  4. Trigger a drop pod (a Firestorm mission, or any scenario that reinforces by pod). droppod.cpp changed how the pod holds itself while handing its carried locomotor back. Failure mode is a crash at touchdown or a passenger that never unlimbos.
  5. Play until a radar event fires — an ion storm ping, a base-under-attack ping, a harvester-under-attack ping — then save and load. revent.cpp had a double-removal bug on our side that upstream's version fixes; the load path is what changed.
  6. Deploy a hover unit next to a repair bay and watch it move onto the pad, and separately watch a transport unload. Those are the unit.cpp sites that changed: the nullptr NavCom test and the passenger-direction fix.
  7. iOS: launch the bundle and check the mode icon still appears when a sidebar mode is armed, that two-finger scroll still pans, and that a held finger still gives a right click. platform/ did not conflict and the seams are all still wired, but nothing on the touch layer has been run since before the merge.

If any of 1 through 4 misbehaves, that is a resolution defect in this PR and not a pre-existing bug; 5 through 7 are more likely to be fine.

ZivDero and others added 19 commits September 10, 2026 01:33
The queue holds its elements itself rather than pointing into a pool beside it,
so a veinhole monster's frontier travels in a save as the cells it holds and the
save format changes. The sift is spelled out rather than taken from the standard
heap algorithms, which are free to reorder elements of equal score.
…ct model (OpenTS-Developers#140)

Co-authored-by: ZivDero <kirill.andriiashin@gmail.com>
Saves written before this merge no longer load: zone ids widened, a
character buffer now travels as its text, and the listing drops the
player name. The packed version is unchanged, so a stale save fails on
a record length rather than being refused.
@msnow345
msnow345 merged commit f17a8df into main Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants