Fix glTF component decomposition: stop silently destroying lights - #132
Merged
Conversation
A glTF node may carry a mesh, a light, a camera and animation channels at once, while an engine Node holds ONE component (Components is a variant). The decomposition was decided implicitly by the order of the attach calls, and the order was wrong in a way nothing reported: the light was attached while the variant still held Empty, and the following emplace<Animator>() (animated node) or emplace<Mesh>() (static mesh + light) overwrote it. No warning fired — the guard inside the light attach had already passed — so the light simply ceased to exist. ShadowLodMotionDemo lost its authored sun exactly that way. hasDirectionalLight() then returned false and FireEngine seeded its fallback directional over the top, so the scene rendered plausibly under a sun the asset never authored, its sun-swing animation did nothing, and every measurement taken on it described lighting nobody chose. Any glTF with an animated light was affected. The fix is the general rule, not the animated-light case. core/node_component_layout.hpp states it once: precedence by what CANNOT move (Animator > Mesh > Light > Camera for the transform-owning node), everything else on an identity-transform child, so the glTF transform stays on the parent and drives the mesh, light and camera together. planNodeComponents decides the layout; materializeNodeComponentLayout applies it to a real node, creating those children and returning NodeComponentTargets — the node each payload belongs on. The loader's attach sites consume a target and no longer inspect the current variant to decide placement for themselves, because that inspection is how the rule and the code came apart: each site reached its own conclusion and the call order became the real policy. attachCamera and attachLight both took a target as a result. Every direct emplacement now goes through requireEmptyComponent, which throws — so the failure that used to delete content silently terminates at the attachment site naming the node and the component already there. Materialization is Vulkan-free, so CI verifies the production topology rather than a parallel description of it. tests/core/test_node_component_layout.cpp is exhaustive over all sixteen combinations, on both the plan and the materialized nodes: every declared payload is placed exactly once, every target is Empty and no two payloads share a node (the property that makes attach order irrelevant), children exist only where the layout calls for them and each is identity. tests/core/test_gltf_node_decomposition.cpp drives the real loader over a purpose-built fixture end to end and is [.][gpu] — local only, since loadScene needs a real Resources. Three of its five cases are fixes (animated + light, static mesh + light, animated mesh + light); camera + light and animated camera already worked and are regression guards, with the animated camera required to actually MOVE rather than merely exist. tests/scene/test_light.cpp pins the scene-side consequences headlessly: a light on a child of an animator satisfies hasDirectionalLight (the fallback-sun gate), is gathered exactly once, follows its parent's rotation, and keeps a stable nodeId across frames. Negative-tested both ways. Restoring the old attach order fails 7 assertions — the three broken combinations lose their lights and gatherLights returns 1 of 4. Pointing the light attach back at the node instead of its target now aborts the load with "cannot attach light — the target already holds Animator" rather than proceeding. Affected evidence re-run, selectively. The static ShadowLodDemo budget calibration reproduces to every printed digit (budget 2 at 0.243%, 4 at 0.356%), as it must: its sun sits on a node with no animation and was never dropped. The ShadowLodMotionDemo dead-band sweep, which did run under a fallback sun, is re-measured with the sun genuinely swinging: 0.50 / 0.16 / 0.16 transitions per 100 frames at ratio 1.0 / 0.75 / 0.5, and 0.00 reversals at every ratio. The rates are 3, 1 and 1 raw events over ~600 frames — counting noise — and the reversal column, the only one that can justify a dead band, is unchanged at zero, so kShadowLodCoarsenRatio stays 1.0. One recorded claim was false and is corrected in constants.hpp: an earlier reversal count was attributed to "a caster walking L1 -> L2 and back as the sun swung", which cannot have happened, because under this defect the sun never moved. It was the moving caster crossing a threshold. The finding is unchanged. docs/shadowplans.md qualifies the SH-06 fixed-depth attribution in place. The half-ellipse was observed under the fallback sun, and sweeping the caster's whole animation range through CascadeReceiverFit::fit -> fitLegacyCascadeDepth -> placeCaster finds no pose where it is clipped by a cascade near plane — closest approach 20.7 m under the fallback sun, 30.8 m under the authored one, with the sweep grounded against the engine's own logged cascade-0 fit and a planted behind-the-plane caster proving it can see a clip. The symptom is real; its attribution is not, and re-diagnosis waits for a trace under corrected lighting.
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.
A glTF node may carry a mesh, a light, a camera and animation channels at once, while an engine Node holds ONE component (Components is a variant). The decomposition was decided implicitly by the order of the attach calls, and the order was wrong in a way nothing reported: the light was attached while the variant still held Empty, and the following emplace() (animated node) or emplace() (static mesh + light) overwrote it. No warning fired — the guard inside the light attach had already passed — so the light simply ceased to exist.
ShadowLodMotionDemo lost its authored sun exactly that way. hasDirectionalLight() then returned false and FireEngine seeded its fallback directional over the top, so the scene rendered plausibly under a sun the asset never authored, its sun-swing animation did nothing, and every measurement taken on it described lighting nobody chose. Any glTF with an animated light was affected.
The fix is the general rule, not the animated-light case. core/node_component_layout.hpp states it once: precedence by what CANNOT move (Animator > Mesh > Light > Camera for the transform-owning node), everything else on an identity-transform child, so the glTF transform stays on the parent and drives the mesh, light and camera together.
planNodeComponents decides the layout; materializeNodeComponentLayout applies it to a real node, creating those children and returning NodeComponentTargets — the node each payload belongs on. The loader's attach sites consume a target and no longer inspect the current variant to decide placement for themselves, because that inspection is how the rule and the code came apart: each site reached its own conclusion and the call order became the real policy. attachCamera and attachLight both took a target as a result. Every direct emplacement now goes through requireEmptyComponent, which throws — so the failure that used to delete content silently terminates at the attachment site naming the node and the component already there.
Materialization is Vulkan-free, so CI verifies the production topology rather than a parallel description of it.
tests/core/test_node_component_layout.cpp is exhaustive over all sixteen combinations, on both the plan and the materialized nodes: every declared payload is placed exactly once, every target is Empty and no two payloads share a node (the property that makes attach order irrelevant), children exist only where the layout calls for them and each is identity. tests/core/test_gltf_node_decomposition.cpp drives the real loader over a purpose-built fixture end to end and is [.][gpu] — local only, since loadScene needs a real Resources. Three of its five cases are fixes (animated + light, static mesh + light, animated mesh + light); camera + light and animated camera already worked and are regression guards, with the animated camera required to actually MOVE rather than merely exist. tests/scene/test_light.cpp pins the scene-side consequences headlessly: a light on a child of an animator satisfies hasDirectionalLight (the fallback-sun gate), is gathered exactly once, follows its parent's rotation, and keeps a stable nodeId across frames.
Negative-tested both ways. Restoring the old attach order fails 7 assertions — the three broken combinations lose their lights and gatherLights returns 1 of 4. Pointing the light attach back at the node instead of its target now aborts the load with "cannot attach light — the target already holds Animator" rather than proceeding.
Affected evidence re-run, selectively. The static ShadowLodDemo budget calibration reproduces to every printed digit (budget 2 at 0.243%, 4 at 0.356%), as it must: its sun sits on a node with no animation and was never dropped. The ShadowLodMotionDemo dead-band sweep, which did run under a fallback sun, is re-measured with the sun genuinely swinging: 0.50 / 0.16 / 0.16 transitions per 100 frames at ratio 1.0 / 0.75 / 0.5, and 0.00 reversals at every ratio. The rates are 3, 1 and 1 raw events over ~600 frames — counting noise — and the reversal column, the only one that can justify a dead band, is unchanged at zero, so kShadowLodCoarsenRatio stays 1.0.
One recorded claim was false and is corrected in constants.hpp: an earlier reversal count was attributed to "a caster walking L1 -> L2 and back as the sun swung", which cannot have happened, because under this defect the sun never moved. It was the moving caster crossing a threshold. The finding is unchanged.
docs/shadowplans.md qualifies the SH-06 fixed-depth attribution in place. The half-ellipse was observed under the fallback sun, and sweeping the caster's whole animation range through CascadeReceiverFit::fit -> fitLegacyCascadeDepth -> placeCaster finds no pose where it is clipped by a cascade near plane — closest approach 20.7 m under the fallback sun, 30.8 m under the authored one, with the sweep grounded against the engine's own logged cascade-0 fit and a planted behind-the-plane caster proving it can see a clip. The symptom is real; its attribution is not, and re-diagnosis waits for a trace under corrected lighting.