Node teardown should follow the model's lifetime (ORC reclaim), not be eagerly triggered by collection-removal. Today the node controller's removed watchers call remove_from_scene → unit.destroy, so removing a unit from a collection destroys it. Decoupling this makes moving a unit between collections natural and lets us delete the interim TRANSFERRING flag (see adopt work).
Target model (main thread)
- add → attach: instance the node only if the model has none yet, then insert into the scene tree.
- remove → detach from the tree (do not free — the node stays held by
unit.node).
- model out of scope (ORC reclaim) → free node.
A move is then just detach-then-attach; no move-vs-delete detection needed.
Mechanism for "model out of scope → free node"
Lifetime.finish() is explicit-only (wrong trigger). The out-of-scope signal is ORC reclaim — i.e. a Nim destructor. Recommended: a small handle field on Model whose =destroy records the dead node into a pending_dead_nodes list (touches nothing else), drained by the node controller on tick (main thread) → queue_free. This mirrors ed's RefHandle→pending_dead_refs→prune_dead_refs pattern and is the symmetric twin of the existing drain_pending add-queue. Avoids calling into Godot from inside an ORC destructor.
unit.destroy (ed teardown + DESTROY sync + owned voxel-table teardown) still runs explicitly on true deletes — clear_all, delete, claim_name, level reload. Those call sites need auditing so they call unit.destroy directly rather than relying on removed → remove_from_scene → destroy.
Why it's safe thread-wise
The node-bearing unit is main-thread-local (worker has a separate, node-less unit object; state/current_build/previous_build are threadvars), so its reclaim — and queue_free — happen on the main thread.
Status / done so far
- Branch
adopt-lifecycle: node.model is now a {.cursor.} (breaks the unit.node ⇄ node.model cycle so a unit leaving all collections can be ORC-reclaimed). First step.
- Full design + findings:
docs/notes/adopt-destructor-teardown.md.
Follow-ups this unblocks
Test prerequisite
nim test_world corrupts tracked tests/worlds/ sources on branches without PR #62's --temp-workdir isolation fix — land/cherry-pick that before in-world validation. Recover with git checkout -- tests/worlds/.
Node teardown should follow the model's lifetime (ORC reclaim), not be eagerly triggered by collection-removal. Today the node controller's
removedwatchers callremove_from_scene→unit.destroy, so removing a unit from a collection destroys it. Decoupling this makes moving a unit between collections natural and lets us delete the interimTRANSFERRINGflag (see adopt work).Target model (main thread)
unit.node).A move is then just detach-then-attach; no move-vs-delete detection needed.
Mechanism for "model out of scope → free node"
Lifetime.finish()is explicit-only (wrong trigger). The out-of-scope signal is ORC reclaim — i.e. a Nim destructor. Recommended: a small handle field onModelwhose=destroyrecords the dead node into apending_dead_nodeslist (touches nothing else), drained by the node controller on tick (main thread) →queue_free. This mirrors ed'sRefHandle→pending_dead_refs→prune_dead_refspattern and is the symmetric twin of the existingdrain_pendingadd-queue. Avoids calling into Godot from inside an ORC destructor.unit.destroy(ed teardown + DESTROY sync + owned voxel-table teardown) still runs explicitly on true deletes —clear_all,delete,claim_name, level reload. Those call sites need auditing so they callunit.destroydirectly rather than relying onremoved → remove_from_scene → destroy.Why it's safe thread-wise
The node-bearing unit is main-thread-local (worker has a separate, node-less unit object;
state/current_build/previous_buildare threadvars), so its reclaim — andqueue_free— happen on the main thread.Status / done so far
adopt-lifecycle:node.modelis now a{.cursor.}(breaks theunit.node⇄node.modelcycle so a unit leaving all collections can be ORC-reclaimed). First step.docs/notes/adopt-destructor-teardown.md.Follow-ups this unblocks
TRANSFERRINGflag once teardown is destructor-driven.set_ownertransfer).Test prerequisite
nim test_worldcorrupts trackedtests/worlds/sources on branches without PR #62's--temp-workdirisolation fix — land/cherry-pick that before in-world validation. Recover withgit checkout -- tests/worlds/.