fix(player): re-borrow the SSAB buffer after an in-place reload - #253
Merged
Conversation
A re-import reloads a .ssab into the same Resource object, freeing the buffer the runtime borrowed. Only the player's own resource was watched for "changed"; the external .ssab files Instance children borrow had no watcher, so those children kept reading freed memory. Track a generation on SSABResource and verify it, for the player and its Instance children, before each step or draw.
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.
Description
Re-importing a project while a
SpriteStudioPlayer2Dis in the scene could crash the editor. Instance parts were required to reproduce it.The runtime borrows a
.ssabbuffer rather than copying it, and that borrow was outliving the buffer.Root cause
SSImporter::_refresh_cached_output(ss_importer.cpp:846) refreshes every generated.ssabby callingload_from_fileon the resource already in the cache, then emittingchanged.SSABResource::load_from_filereassignsbinary, so the object survives but its buffer is freed and replaced._fetchAnimationhands that buffer toss_resource_create_borrowand keeps_currentAnimationDatapointing into the same FlatBuffer, so both dangle the moment it is replaced.The player's own resource is covered: the Node2D connects
changedandonSSABReloadedre-borrows. The gap is the external.ssabfiles — sibling packs loaded by_load_external_ssabsso Instance children can resolveref_anime_pack— which nothing connectschangedon.Whether it crashed came down to refresh order.
_record_ssabs_in_dir(:820) pushes the generated files inDirAccessenumeration order, so when the parent pack sorts before the external one (InstancebeforeInstance_SourceAnime, for example) the children are rebuilt while the external still holds its old buffer, and are left pointing at freed memory when it is refreshed a moment later. Freed-but-not-yet-reused memory usually still reads, which is why it only crashed sometimes.This is not module-only.
copy_fromis (#ifndef SPRITESTUDIO_GODOT_EXTENSION), but the importer path above compiles in both builds.Fix
SSABResourcenow carries a generation, bumped whereverbinaryis replaced —load_from_fileandcopy_from. Inload_from_fileit is bumped up-front so the failure paths, which leavebinaryreassigned or cleared, invalidate outstanding borrows as well.SsInternalPlayerrecords the generation it borrowed and checks it — for itself and, recursively, for its Instance children — before it steps or draws, rebuilding throughonSSABReloadedon a mismatch. Two call sites:update, ahead of theis_playingearly-return so a stopped player still notices, and_seek_and_redraw, which draws without going throughupdate. The recursion walks children that already exist, not the reference graph they were built from, so it terminates regardless of how the data references itself.Comparing
get_data_ptr()instead would not be enough: freeing and re-allocating a same-sized buffer frequently lands on the same address.Alternative considered
Connecting
changedon the external resources as well. Rejected on two counts:copy_fromemits nochangedat all, and the set of resources to watch changes with every animation switch and every level of Instance nesting, so it re-creates the same "one missed spot equals a dangling borrow" failure this bug is an instance of. Checking at the point of use keeps the invariant local to the borrow.Verification
No SDK change; the submodule pin is untouched.