Skip to content

fix: let parents own their children and search the whole scene hierarchy - #193

Merged
CanReader merged 4 commits into
mainfrom
fix/scene-hierarchy-ownership
Oct 8, 2026
Merged

CanReader merged 4 commits into
mainfrom
fix/scene-hierarchy-ownership

Conversation

@CanReader

@CanReader CanReader commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner

What this changes

A GameObject now owns its children unless a child was itself handed to a scene with AddObject(). Deleting a parent deletes the children it owns, so a hierarchy built with SetParent() only needs its root added. Lights and colliders anywhere in that hierarchy are registered and unregistered with the scene, the FindObjectBy* queries search it, and Object keeps the name you give it instead of appending its ID.

Why

  1. ~GameObject() set every child's parent to null and walked away, and the scene only deletes what is in Objects. ModelLoader builds a root and attaches one GameObject per submesh with SetParent(), and games only AddObject() the root. So every submesh, with its mesh, material and transform buffers, leaked on RemoveObject() and on Unload(). 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 in Objects is 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 way DestroyObject() 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.
  2. AddObject() only registered a light if the object itself was one, so a light under a model root never reached the LightManager. 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's Initialize() created the LightManager were never registered, and DestroyAllObjects() on Unload() never unregistered anything, leaving freed lights and colliders in the LightManager and PhysicsWorld, 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.

  3. FindObjectByName, FindObjectByID and FindObjectsByTag only looked at Objects, so no submesh or child could ever be found. They now search each object's owned hierarchy. The debug collider drawing walks it too.

  4. The Object constructor did SetName(name + "-" + id), so a GameObject("Crate") was really called Crate-15 and FindObjectByName("Crate") could never match (the example in the SceneBase docs did not work). The name is now kept as given. GetDebugName() returns the old name-id form 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 what ModelLoader did by hand with its own InitializeRecursive, 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. ModelLoader builds the whole tree before returning, so it is not affected. The skeleton and clips ModelLoader allocates for animated models are still not owned by anything, that is a separate leak.

How it was verified

  • Builds on Linux
  • Builds on Windows
  • Ran the sample scene on Vulkan
  • Ran the sample scene on OpenGL

Small harness linked against libEngined.so under ASan, with fake texture, shader and buffer factories. It loads a two-mesh OBJ through ModelLoader, adds the root to a scene and unloads, looks up a child by name, adds a root with a PointLight child 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:

model: children=2 buffers=8
find child 'A-1': null
after unload: buffers=8
name 'Crate-15', find Crate: null
child light registered: lights=0
...
SUMMARY: AddressSanitizer: 5208 byte(s) leaked in 48 allocation(s).

After:

model: children=2 buffers=8
find child 'A': found
after unload: buffers=0
name 'Crate', find Crate: found
child light registered: lights=1
after destroy: lights=0
destroy parent with added child: objs=1 count=1
remove parent with pending child: objs=1
after unload: objs=0
clean exit

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 the MainScene unload 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

  • Public headers documented with /// comments
  • No new includes from src/ or include/private/ in public headers
  • No game-specific vocabulary added to engine API

@CanReader
CanReader changed the base branch from fix/scene-object-lifecycle to main October 4, 2026 20:07
@CanReader
CanReader merged commit dd39889 into main Oct 8, 2026
12 checks passed
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.

1 participant