Merge upstream's save container and object model into the port - #3
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merges the 17 upstream commits from
OpenTS-Developers/OpenTSmain(0281b88..945e858) into the macOS/iOS port. 49 conflicts, 39 of them undercode/.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 as93613dbandf43aae0, developed independently. In most conflicted files the two sides were 90% identical —code/savefile.cppdiffered by 387 lines out of 571,code/drive.cppby 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
code/savefile.cpp/.hstd::uint32_tthroughout;CRC::Memoryinstead of a private CRC-32 table; writes the header, table and payload without copying them into one image first; Win32 file handles instead ofstd::FILE *code/savestream.hBoundScope(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 achar[N]serializercode/saveload.cpp/.hLoad_Vector<T>andLoad_Object_As<T>, so a record naming a class that does not belong in a heap fails the load; ownership throughstd::unique_ptr; the listing is read from the same pass as the content;std::bad_allocandstd::length_errorare caughtcode/classfactory.*std::unique_ptrcreatorscode/ini.cppClassIDparser instead ofsscanfover a brace-stripped copy, and a doc comment moved onto the function it describescode/mouse.cppint;ZoneAdjacencyand the subzone connection tables moved to std containers; a second-pass count mismatch now fails the save instead of writing an unreadable mapcode/vein.cppPriorityQueueClassbacked by a vector; noGrowthNodesarraycode/revent.cpp~RadarEventClassalready callsRadarEvents.Delete(this), so our extraDelete_Index(i)was removing a second, unrelated event on every iteration. Upstream also bounds the count before allocatingcode/unit.cppfaceto = Direction(passenger).As_Dir256()replaces a self-initialisedDirType direction = direction.Direction(...)(OpenTS-Developers#130)code/droppod.cppcode/swizzle.cppAbandonno longer writes through the registered pointer slots; they were never filledcode/foot.cppPATH_LENGTH_MAX + 1rather than a bare200*10IPiggyback::Begin_Piggybackstd::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 acrossdrive,walk,droppod,building,infantry,unit,foottests/save,docs/SAVE-FORMAT.md,manual/code/blowfish.h#include "win.h", which removes an#ifdef _WIN32fromcode/Kept from this fork
Three lines, all of them port seams that upstream has no reason to carry:
code/taction.cpp—Host_Sleep(1000)and#include "hostclock.h"instead ofSleep(1000).Host_Sleepis the port's portable clock seam, used in eight files; the include was also moved belowalways.h, wheredocs/STYLE.mdputs it.code/techtype.cpp—#include <new>rather than upstream's<new.h>, which is MSVC-only.code/mouse.cpp—MouseClass::Get_Mouse_Frame_Rate, which the iOS mode icon reads.Moved out of
code/and intoplatform/Upstream's
savefile.cppandsaveload.cppcall Win32 entry points the shim did not have. Rather than keep our portable-but-divergent versions, the missing entry points were added toplatform/win32compat, which is where a platform difference belongs and which leavescode/byte-identical to upstream:GetFileSize,FlushFileBuffers,MoveFileExA(kernel.cpp), plusINVALID_FILE_SIZEandMOVEFILE_REPLACE_EXISTING. AHANDLEis already astd::FILE *here, so these are a dozen lines each.GetSystemTimeAsFileTime, which letscode/saveload.cppdrop its own#ifdef _WIN32epoch helper.The save checksum is unchanged by the switch to
CRC::Memory: its table is the standard reflected CRC-32 table,crc ^= 0xFFFFFFFFis~seed, and(crc >> 8) & 0x00FFFFFFiscrc >> 8at 32 bits. Bit-for-bit the same value our private table produced.The assertion guards, and one that fired
All 42 of upstream's
#149guards 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,CompHeaderType8,IsoTileRecord52,IsoTileSet20, MIXFileHeader6, PCXRGB3 andPCX_HEADER128,RGBStruct3,ShapeRecord24,ShapeSet8,VQASN2J12,VQAHeader42. 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 issizeof(data) == 12, whose twin issizeof(SNJ2Struct) == 12three lines above it.One upstream guard fired, and it found a real port defect.
static_assert(sizeof(MSBitmap) == 58)incode/srfcache.cppfailed at 86 bytes.platform/win32compathastypedef long LONG, which is 8 bytes on LP64, so itsBITMAPINFOHEADERwas 64 bytes rather than 40 and every field afterbiSizesat at the wrong offset.SurfaceCacheClass::CacheBMPcasts a raw.BMPfile 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:tagBITMAPINFOHEADERnames a newLONG32for the four fields the format fixes at four bytes, withstatic_assert(sizeof(BITMAPINFOHEADER) == 40)beside it.dsurface.cpp,wincursor.cppandgdi.cppuse the same struct in-process and are consistent with the narrower one.Things upstream would probably rather I had not done
OPENTS_ARCHfalls back toCMAKE_SYSTEM_PROCESSOR. Upstream'sd1d7c06readsCMAKE_CXX_COMPILER_ARCHITECTURE_ID, which only MSVC sets, so every native and cross build would stampunknown. One extraif.enable_language(RC)stays insideif(WIN32). Upstream unguarded it; no other toolchain here has a resource compiler.code/taction.cpp,code/techtype.cpp,code/mouse.cppcarry the three port lines above.tests/save/savetest.cppby 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 whetherlzo1x_decompress_safestill 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_VERSIONstill 1, same header layout, same CRC — but the game-state stream moved:unsigned shorttoint, and the subzone connection tables changed shape.char[N]member now travels as a length and its text; before, upstream's genericT (&)[N]overload wrote all N bytes raw.PIDSI_PLAYER_NAME1orPIDSI_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_VERSIONwould 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.mdnow 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:
(Only the
OpenTStarget: 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 tenWin_*/Video_*/UI_Mode_Icon_*port seams are still wired to the same call sites;code/netdlg2.cppis byte-identical to its pre-merge version, so all fourLobby_Seat_Is_Validsites 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.
tests/save,tests/priorityqueueand the other 36 harnesses are MSVC-only and do not build here.manual/tools/manage.py checkandcommands.pycould not run:PyYAMLis 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.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.
platform/file entry points (MoveFileExAis what commits the.tmpinto 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 inDebug/DEBUG_*.LOG, not crash and not load wrong.unsigned shorttointin 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.Begin_Piggybackchanged signature everywhere, and the war-factory exit inbuilding.cppand the two infantry sites ininfantry.cppare 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.droppod.cppchanged how the pod holds itself while handing its carried locomotor back. Failure mode is a crash at touchdown or a passenger that never unlimbos.revent.cpphad a double-removal bug on our side that upstream's version fixes; the load path is what changed.unit.cppsites that changed: thenullptrNavCom test and the passenger-direction fix.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.