Repository navigation
fix: let parents own their children and search the whole scene hierarchy - #193
Merged
Merged
Conversation
5 of 7 tasks
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.
What this changes
A
GameObjectnow owns its children unless a child was itself handed to a scene withAddObject(). Deleting a parent deletes the children it owns, so a hierarchy built withSetParent()only needs its root added. Lights and colliders anywhere in that hierarchy are registered and unregistered with the scene, theFindObjectBy*queries search it, andObjectkeeps the name you give it instead of appending its ID.Why
~GameObject()set every child's parent to null and walked away, and the scene only deletes what is inObjects.ModelLoaderbuilds a root and attaches oneGameObjectper submesh withSetParent(), and games onlyAddObject()the root. So every submesh, with its mesh, material and transform buffers, leaked onRemoveObject()and onUnload(). SleakSims hits this every time a mannequin is removed.The rule now: a parent deletes each child that is not
IsOwnedByScene().AddObject()sets that flag, so a child that is also inObjectsis left for the scene and never deleted twice. To keep that true on the scene side:ProcessPendingDestroy()detaches every queued object from its parent before deleting any of them, so nothing in the queue (Fix: RemoveObject double-free on an object already pending destroy #179's fix) gets deleted through a parent and then again from the queue.RemoveObject()removes scene owned children first, then drops anything it is about to delete from the pending queue, not just the object itself. It now takes the whole hierarchy with it, the same wayDestroyObject()already did.DestroyAllObjects()detaches everything first for the same reason.SetParent()refuses to parent an object to its own descendant, since that cycle would now recurse forever on delete.AddObject()only registered a light if the object itself was one, so a light under a model root never reached theLightManager. Registration now walks the owned hierarchy (lights and colliders both), and so does every unregister path. Two related holes closed while there: lights added before the scene'sInitialize()created theLightManagerwere never registered, andDestroyAllObjects()onUnload()never unregistered anything, leaving freed lights and colliders in theLightManagerandPhysicsWorld, which both survive an unload. Collider unregistration used to walk all descendants including scene owned ones, which could unregister a child that outlives its parent. It now stops at scene owned children, which unregister themselves.FindObjectByName,FindObjectByIDandFindObjectsByTagonly looked atObjects, so no submesh or child could ever be found. They now search each object's owned hierarchy. The debug collider drawing walks it too.The
Objectconstructor didSetName(name + "-" + id), so aGameObject("Crate")was really calledCrate-15andFindObjectByName("Crate")could never match (the example in theSceneBasedocs did not work). The name is now kept as given.GetDebugName()returns the oldname-idform for logs where names repeat. I checked SleakCraft, SleakSims, Empty and FPP for anything relying on the suffix, and nothing does. The member init order also matched the declaration order the wrong way round, that is fixed too.GameObject::Initialize()now also initializes its children, which is whatModelLoaderdid by hand with its ownInitializeRecursive, so that helper is gone.Not covered here: children attached with
SetParent()after their root is already in a scene are not registered with lights and physics until something re-adds them.ModelLoaderbuilds the whole tree before returning, so it is not affected. The skeleton and clipsModelLoaderallocates for animated models are still not owned by anything, that is a separate leak.How it was verified
Small harness linked against
libEngined.sounder ASan, with fake texture, shader and buffer factories. It loads a two-mesh OBJ throughModelLoader, adds the root to a scene and unloads, looks up a child by name, adds a root with aPointLightchild and destroys it, destroys a parent whose child is also scene owned, removes a parent whose child is queued for destroy, and unloads a scene after attempting a parent cycle.Before:
After:
No ASan errors or leak reports after.
Also rebuilt SleakCraft against it and ran it on my desktop session with
-world citest -seed 42 -rd 4: 90s on OpenGL (NVIDIA GL, the xvfb wrapper did not take effect under Wayland) and 60s on Vulkan. Both create the world and run until the timeout with no errors, and on Vulkan the shutdown goes through theMainSceneunload cleanly. The only validation message there is the shader module and pipeline layout leak from #131, which is unrelated. Not built or run on Windows.Checklist
///commentssrc/orinclude/private/in public headers