Repository navigation
Port: stop blending from motion sets whose memory was released - #125
Conversation
|
20261019-113821-e7bb.zip the bad I'm running this PR on top of master now. only conflict is |
|
found a problem with this one after playing more. in town every location change (teleporting or just walking into a new area) leaves Max or Monica frozen in one pose sliding around until you switch characters. it's what I'm running locally is an alloc that doesn't report, and the two u_long128 *MemoryAllocKept(mgCMemory &memory, int size) {
g_keeping_contents = true;
u_long128 *data = memory.Alloc(size);
g_keeping_contents = false;
return data;
}
|
The game frees characters, effect-script copies and their motions by resetting arena memory, so character_motion never hears that a motion set or frame tree is gone. Its registry kept the released set's actions and the record of what each frame tree last posed. When a new frame tree was built at the same address, such as the next effect-script character copied into the reused work buffer, its first blend took the old tree's pose as the source and walked channel lists that now held other data, crashing in BlendActions. Constructing a frame now drops the pose recorded for its address, as it already drops the frame's native conventions. Registering a motion set also stamps its anchor, so a set whose memory has been reused, or registered again at the same address, no longer resolves: the blend falls back to the target set, and an unknown set poses nothing.
The anchor stamp only shows that the anchor's own bytes survived. BuildMotion allocates the anchor after the action channels, so an arena reset that hands the front of the run out again overwrites the channels and leaves the anchor, stamp included, intact. A frame tree that outlives the reset still finds the set through its recorded pose and blends through the overwritten channels. Registering a motion set now records the arena run that holds its channels and anchor. The port's mgCMemory reports each range it hands out or writes a block header over: stack allocations, placement growth of the last run, the header of a stack-mode block claimed from the heap and the headers of a heap set up again. Any set whose run overlaps that range is forgotten. Hooking the release points themselves would miss most of them. stReset is inline in the retail header, many game functions write stack_used directly to rewind an arena, and heap Free only unlinks the block. None of them touch the contents, which are only overwritten once the arena hands the memory out again, so that is where the registry hears of it. The stamp stays as a guard against an anchor overwritten without going through an arena.
Not every write over released motion memory goes through a hooked allocation. File loads write the whole sector-padded destination, often at the top of an arena, and the caller reserves only the file's size afterwards, as MenuCommonReadData does. Channels in the padding were overwritten while the anchor past the sector kept its stamp. Setting up a heap over a buffer outside the arenas copies that buffer into its arena root, and binding the same buffer again maps the same root, so the copy could overwrite an interior set's channels while an anchor in the root's headroom survived. The main-thread loads now report their destination before writing it: the padded size of a data or save file read, a Luau script handle, and a file cache copy. The worker threads that validate script timing read through ReadUnregistered, which stays untouched. SetHeapMem and the stack buffer import report the span they copy over. A set rejected by its stamp is now erased rather than skipped, and registering a set again forgets the old one's poses, so stale entries do not pile up in the registry.
SetDataPacket runs as the town loads and on every map change. It rewinds the town's character buffer and reserves the controlled character's block again with ControlCharaBuff.Alloc(FixCharaBuffSize), while the character stays loaded there. The arena reports every range it hands out as overwritten, so the motion registry forgot the character's sets, and Max or Monica stood in the bind pose and slid around until a character change loaded them again. AllocQuadwords now takes whether the memory it hands out is released or kept, and MemoryAllocKept reserves kept memory without reporting it. A unit fixup routes both re-reservations in SetDataPacket through it. The other places that rewind an arena and reserve it again, such as GetCharaMemAllocPtr for set-up, character change and Monster Transform, load new data straight after, so they still release the old sets. Reported by FredTheSlug, who traced it to SetDataPacket.
6370195 to
ba88d40
Compare
|
Thanks Fred, both of these were spot on. Conflict with #122: rebased onto master (c27f3d0) and resolved Frozen Max/Monica in town: reproduced headless with The fix follows your approach, without the global flag. Tests:
Thanks for running it on top of master and for tracking this one down. |
Before
character_motionkeeps a registry of motion sets keyed by their channel anchor, and a record of the action each frame tree last posed. The game releases characters, effect-script copies and their motions by resetting or rewinding arena memory, or by unlinking heap blocks, and never tells the registry. Stale entries survived. A frame tree built at a reused address, or a surviving frame tree whose motion memory was reused, took the old pose as its blend source and walked channel lists that now held other data. This is the failure mechanism the tests below reproduce. It matches a playtest crash inBlendActionswhen shooting a frog after switching guns, and FredTheSlug's crash in Rainbow Butterfly Wood through an effect-script character, but neither crash has been reproduced in game.After
mgCMemoryreports every range it hands out (stack allocations, placement growth of the last run), every block header it writes (heap setup, stack-mode claims), and the span a heap or stack buffer import copies over;MenuCommonReadDataload at the top of an arena and reserve only the unpadded size afterwards.SetDataPacket) rewinds the character buffer and reserves the controlled character's block again while the character stays loaded there. Those two reservations go throughMemoryAllocKept, by a unit fixup, and leave the registry alone. The other paths I traced—character setup and change, Monster Transform, dungeon and town initialization, and menu loading—allocate for replacement data or rebuild their characters, so they retain normal invalidation. I found no second reservation that needs to preserve a live motion set.How
Hooking the release points was not workable:
stResetis inline in the retail header, many game functions rewind by writingstack_useddirectly, and heapFreeonly unlinks. None of them touch the contents, so the registry hears about memory when it is about to be overwritten instead. The worker threads that validate script timing read throughReadUnregistered, which is unchanged; only main-thread wrappers report.stAllocTestpreviews do not invalidate anything.The registry is scanned linearly on each report. Measured with temporary counters (not committed) over 18,000 headless ticks each:
dungeon:0:2, gun and wrench)town:10, walking and talking)The per-report times include the two clock reads around each call. The maximum ticks are loads. Ranges of registered sets are disjoint, since an allocation over one forgets it, so the registry stays bounded by arena memory. I left the scan linear.
Tests
character_motion_test.cpp(merged with Port: fix the Help and Monica inventory crashes and effects stuck bright #122's cases; the plain cases use itsCharacterMotionfixture). The arena cases lay memory out asBuildMotiondoes: channels first, anchor at the end of the run.MemoryAllocKept: the set keeps resolving, and a later reset that hands the memory out releases it.dataread_test.cpp: a 10-byte file loaded at the top of a reset arena, with the caller reserving only the file's size, overwrites channels in the sector padding while the anchor past 2 KiB survives; the set must be forgotten.test_header_fixups.py: bothControlCharaBuff.Alloc(FixCharaBuffSize)calls ineditloop.cppbecomeMemoryAllocKept, and the packet buffers keepAlloc.AllocationPastMotionStorageKeepsSetis the negative control and passes either way.cteston Windows: 1858 passed, 54 skipped, 1 failed, and 1 disabled.GeoramaParts.PaintedFencesMatchRetailRecursionfails intermittently: the same test binary can pass and fail repeated runs. It also fails in camera-only branches based onc27f3d041that do not contain this PR, at the reference fence-connectivity assertion beforePaintFenceis called. The underlying nondeterminism is not diagnosed, and a clean-master binary was not tested in this review.origin/masterand this branch, with screenshots at two frames each compared byte for byte. The replays cover town walking, the Morton's shop round trip, the world map, menus, invention, camera, the Monster Transform menu, the dungeon floor, menus and gun, Ridepod, the Mt. Gundor carry, the quick change to Monica, Monica's opening, Ocean's Roar, Georama, and dungeon to town with Escape Powder. All are identical. Without theSetDataPacketfixup,--jump town:10 --fast-loadwithtools/input/area-town-building.inputshows Max in his bind pose from the first controllable frame, while master animates him. Headless and screenshot runs disable the render thread, so these runs cover that configuration only.