From 3313061b4dbc3dd4d53dfd42f8344f8870a66ef0 Mon Sep 17 00:00:00 2001 From: Nick Newson Date: Sat, 1 Aug 2026 14:39:38 +0100 Subject: [PATCH] Fix glTF component decomposition: stop silently destroying lights MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CMakeLists.txt | 3 + docs/acceptance-testing.md | 6 +- docs/onboarding.md | 19 ++ docs/review-order.md | 1 + docs/shadowplans.md | 14 + .../core/node_component_layout.hpp | 93 +++++++ include/fire_engine/render/constants.hpp | 27 +- src/core/gltf_loader_nodes.cpp | 159 +++++------ src/core/node_component_layout.cpp | 90 +++++++ .../assets/node_component_decomposition.gltf | 255 ++++++++++++++++++ tests/core/test_gltf_node_decomposition.cpp | 228 ++++++++++++++++ tests/core/test_node_component_layout.cpp | 209 ++++++++++++++ tests/scene/test_light.cpp | 92 +++++++ 13 files changed, 1111 insertions(+), 85 deletions(-) create mode 100644 include/fire_engine/core/node_component_layout.hpp create mode 100644 src/core/node_component_layout.cpp create mode 100644 tests/assets/node_component_decomposition.gltf create mode 100644 tests/core/test_gltf_node_decomposition.cpp create mode 100644 tests/core/test_node_component_layout.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 7b7af01f..13decdb8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -79,6 +79,7 @@ add_library(fireengine SHARED src/collision/dynamic_aabb_tree_broad_phase.cpp src/collision/narrow_phase.cpp src/core/convex_hull_builder.cpp + src/core/node_component_layout.cpp src/core/node_id.cpp src/core/gltf_loader_assets.cpp src/core/gltf_loader_extras.cpp @@ -440,6 +441,8 @@ add_executable(test_fire_engine tests/core/test_shader_loader.cpp tests/core/test_system.cpp tests/core/test_gltf_loader.cpp + tests/core/test_node_component_layout.cpp + tests/core/test_gltf_node_decomposition.cpp tests/core/test_convex_hull_builder.cpp tests/core/test_tangent_generator.cpp tests/input/test_camera_state.cpp diff --git a/docs/acceptance-testing.md b/docs/acceptance-testing.md index 6eb0194a..84657d76 100644 --- a/docs/acceptance-testing.md +++ b/docs/acceptance-testing.md @@ -477,7 +477,11 @@ at full detail. ./fireEngineApp shadow_lod/ShadowLodMotionDemo.gltf nightbox.hdr --overlay ``` Same content, plus a caster crossing the cascade bands, a swinging sun, a swinging skinned limb and -a pulsing morph. **This is not a screenshot reference** — an animated frame has no reproducible +a pulsing morph. **The sun genuinely swings only since 2026-08-01**: before the glTF loader stopped +dropping lights on animated nodes, this scene's authored sun was replaced by the engine's fallback +directional, so shadows pointed a different way and never moved with the light. If you remember this +loop looking static in its lighting, that is why — and any earlier note taken against it describes +the fallback sun. **This is not a screenshot reference** — an animated frame has no reproducible timestamp. Watch a full loop for: shadow silhouettes popping as the caster crosses a cascade boundary, level chatter (a shadow flickering between two detail levels), and the skinned limb's shadow separating from the limb. Report what you saw; there is no numeric gate until SH-02 defines diff --git a/docs/onboarding.md b/docs/onboarding.md index a8999d72..bad6eea9 100644 --- a/docs/onboarding.md +++ b/docs/onboarding.md @@ -909,6 +909,25 @@ the same change — most have a test or guard that will catch you, but not all. `LodDisabled`, which is a user's toggle, and the panel would then explain a safety fallback with somebody else's reason. There is deliberately **no shadow-proxy setter** — see `Object`'s header for what a validated one must enforce before it comes back. +- **A glTF node's contents are decomposed by ONE rule, and the light is attached last.** A `Node` + holds a single component (`Components` is a variant), while a glTF node may carry a mesh, a light, + a camera and animation at once. `core/node_component_layout.hpp` states the rule — + Animator > Mesh > Light > Camera for the transform-owning node, everything else on an + identity-transform `_Light` / `_Mesh` / `_Camera` child — and `gltf_loader_nodes.cpp` follows + it — through `materializeNodeComponentLayout`, which creates the children and returns the target + node for the mesh, the light and the camera. **The attach sites consume those targets and must not + inspect the current variant to decide placement for themselves**: that inspection is how the rule + and the code came apart in the first place, since each site then reached its own conclusion and + the ORDER of the calls became the real policy. Every direct emplacement goes through + `requireEmptyComponent`, which throws — the previous failure was silent: the light was attached + while the variant still held `Empty` and then destroyed by `emplace()` or + `emplace()` with NO warning (the guard inside the attach had already passed). `ShadowLodMotionDemo` lost its authored sun that way, + `hasDirectionalLight()` went false, and `FireEngine` seeded a fallback directional over the top — + so the scene rendered, plausibly, under the wrong sun for every measurement taken on it. If you + add a component kind, add it to the rule and to `tests/core/test_node_component_layout.cpp`, which + is exhaustive over the combinations and over the materialised topology (both GPU-free, so CI runs + them); `tests/core/test_gltf_node_decomposition.cpp` is the end-to-end confirmation through the + real loader and is `[.][gpu]`, so it runs locally only. - **A cascade's texel size comes back OUT of the fit, never recomputed** (SH-06). `fitCascadeReceiver` (`render/cascade_fit.hpp`) returns `worldPerTexel` alongside the geometry it snapped to, and `Renderer::computeShadowCascades` hands that value straight to diff --git a/docs/review-order.md b/docs/review-order.md index 1ecea61a..3858ff27 100644 --- a/docs/review-order.md +++ b/docs/review-order.md @@ -42,6 +42,7 @@ Read these first when a change touches build configuration, CI, or local tooling | File | Pay attention to | |---|---| | `math/constants.hpp` | Just π/epsilon constants — orient quickly. | +| `core/node_component_layout.hpp` + `.cpp` | Tiny but load-bearing: the rule deciding which of a glTF node's contents (Animator / Mesh / Light / Camera) owns the engine node and which move to identity-transform children. Exists because a `Node` holds ONE component while a glTF node may carry several, and the previous implicit rule — attach order — silently destroyed lights (`emplace` / `emplace` over an already-attached `Light`, no warning). Precedence is by what cannot move: an Animator must stay on the animated node, or its children stop following the animation. `materializeNodeComponentLayout` applies the plan to a real node — creating the identity children and returning the target for each payload — so the loader's attach sites consume a node rather than deciding placement from the current variant; that is what keeps the rule and the code from drifting, and it is Vulkan-free so CI verifies the actual topology. Exhaustively tested in `tests/core/test_node_component_layout.cpp`. | | `math/vec_base.hpp` | CRTP base for the vec types; compound-assign are primitives, binary ops delegate. | | `math/vec2.hpp` / `vec3.hpp` / `vec4.hpp` | `Vec3` is the workhorse. **`magnitude()`/`normalise()` are deliberately NOT constexpr** (sqrt). `operator==` is **strict bit equality** — use `approxEqual` for tolerance. Vec3↔Vec4 conversions are `explicit` both ways. | | `math/quaternion.hpp` | SLERP, `fromVectors`, Hamilton `operator*`, and `integrate(ω, dt)` (exponential-map orientation integration for the rigid-body solver). Used for all scene rotation; glTF round-trips through this. | diff --git a/docs/shadowplans.md b/docs/shadowplans.md index 5acfad1b..2b4f437b 100644 --- a/docs/shadowplans.md +++ b/docs/shadowplans.md @@ -570,6 +570,20 @@ The cascade XY fit is stable, but its light-space depth currently relies on the `kShadowDepthBackExtend`. A caster farther behind the receiver slice than that constant can be clipped even though its shadow reaches the slice. +**QUALIFIED, 2026-08-01 — do not treat the paragraph below as an established depth-clip case.** Two +things came out of building the reproduction. First, `ShadowLodMotionDemo` was rendering under the +engine's FALLBACK sun: the glTF loader dropped lights on animated nodes, so the authored, swinging +sun never reached the scene (fixed on `gltf-component-decomposition`; see +[`onboarding.md`](onboarding.md) § Cross-File Invariants). Every observation on this scene, including +the one below, was made under lighting the asset did not author. Second, sweeping the caster's whole +animation range through `CascadeReceiverFit::fit` -> `fitLegacyCascadeDepth` -> `placeCaster` finds +NO pose where the moving caster is clipped by a cascade near plane — closest approach 20.7 m under +the fallback sun, 30.8 m under the authored one. The sweep reproduces the engine's own logged +cascade-0 fit to the printed digit and detects a deliberately planted behind-the-plane caster, so the +null is not vacuous. The symptom below is therefore real as an observation but MISATTRIBUTED as +fixed-depth clipping; re-diagnose it under corrected lighting with `placeCaster`, which separates a +depth clip from a footprint miss, before any fixture is frozen. + **Observed, 2026-07-29** (reported from a live run during SH-03 slice 6, then reproduced). On `ShadowLodMotionDemo`, as the moving sphere passes the detail cluster, the top third of its cast shadow disappears: the shadow renders as a half-ellipse with a straight upper edge while the sphere diff --git a/include/fire_engine/core/node_component_layout.hpp b/include/fire_engine/core/node_component_layout.hpp new file mode 100644 index 00000000..838c1bb8 --- /dev/null +++ b/include/fire_engine/core/node_component_layout.hpp @@ -0,0 +1,93 @@ +#pragma once + +#include +#include + +namespace fire_engine +{ + +class Node; + +// How one glTF node's contents are laid out across engine nodes. +// +// A `Node` holds ONE component (`Components` is a variant: Empty/Animator/Camera/Mesh/Light), while +// a glTF node may legitimately carry a mesh, a light, a camera and animation channels at once. The +// engine therefore decomposes such a node into a parent that owns the TRANSFORM plus identity +// children for whatever else it carries. +// +// This was previously 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`, then +// `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 that way and rendered under the engine's +// fallback directional for every measurement taken on it. +// +// Making the layout an explicit value fixes the class of bug rather than the instance: the rule is +// stated once, tested exhaustively, and the loader cannot express "attach A then silently destroy +// it with B". +struct NodeComponentLayout +{ + // What the transform-owning node itself holds. + enum class Primary : std::uint8_t + { + // Nothing of its own — a pure transform, or a node whose only content moved to a child. + Empty, + // Transform animation. The animated node MUST own this: every child inherits the animated + // transform, which is exactly how an animated light or mesh follows its channel. + Animator, + // A static mesh with nothing competing for the slot. + Mesh, + // A light with nothing competing for the slot. + Light, + // A camera on a node that carries nothing else. + Camera, + }; + + Primary primary{Primary::Empty}; + // Each of these means "this content exists and needs its own child node". A child is created + // with an IDENTITY transform: the glTF transform stays on the parent, so animation drives the + // mesh, the light and the camera together rather than any one of them individually. + bool meshOnChild{false}; + bool lightOnChild{false}; + bool cameraOnChild{false}; +}; + +// The decomposition rule. Precedence is by what CANNOT move: an Animator has to sit on the animated +// node, so it wins outright; a mesh takes the node only when no animator needs it; a light takes it +// only when nothing else does. A camera never claims the node when anything else is present, which +// is the behaviour the camera path already had and which this rule now states for all four. +// +// `hasTransformAnim` is specifically TRANSFORM animation (translation/rotation/scale). Weight-only +// animation drives morph targets through the Mesh component and needs no Animator of its own, so a +// weight-animated node still lays out as a plain mesh node. +[[nodiscard]] NodeComponentLayout planNodeComponents(bool hasTransformAnim, bool hasMesh, + bool hasLight, bool hasCamera) noexcept; + +// Where each payload is to be attached, after the layout has been applied to a real node. +// +// The point of returning NODES rather than booleans: an attach site that reads a flag can still +// decide placement for itself, and then the planner's tests pass while production drifts. A site +// handed a target has nothing left to decide — and nothing left to get wrong when a future change +// reorders the calls. +// +// A null pointer means the glTF node did not declare that payload. It is never a fallback: if the +// layout says a light exists, `light` is non-null. +struct NodeComponentTargets +{ + Node* mesh{nullptr}; + Node* light{nullptr}; + Node* camera{nullptr}; +}; + +// Applies a layout to `node`, creating exactly the identity-transform children it calls for, and +// returns the node each payload belongs on. Vulkan-free and scene-only, so the whole topology is +// testable headlessly — the production rule and the production node tree are the same code. +// +// `meshChildName` names the mesh child when one is needed (glTF meshes carry their own names, and +// the loader prefers them); empty falls back to `_Mesh`. Light and camera children are always +// `_Light` / `_Camera`. +[[nodiscard]] NodeComponentTargets +materializeNodeComponentLayout(Node& node, const NodeComponentLayout& layout, + std::string_view meshChildName = {}); + +} // namespace fire_engine diff --git a/include/fire_engine/render/constants.hpp b/include/fire_engine/render/constants.hpp index f8d9f7e7..74e2b5fc 100644 --- a/include/fire_engine/render/constants.hpp +++ b/include/fire_engine/render/constants.hpp @@ -154,6 +154,11 @@ inline constexpr float kPointShadowInfiniteRangeFallback = 100.0f; // The 0.1% threshold was re-applied unchanged, and still selects 1: budget 2 remains at 0.243% and // 4 at 0.356%. Budget 2 becoming eligible once deformable error disappeared was a live possibility // worth checking, and it did not happen. +// +// Re-run 2026-08-01 after the glTF animated-light fix and reproduced to every printed digit. That +// is the expected result rather than a lucky one: the budget half of the sweep runs on the STATIC +// `ShadowLodDemo`, whose sun sits on a node with no animation and was therefore never dropped. The +// dead-band half, which did run under a fallback sun, is re-measured below. inline constexpr float kShadowLodPixelBudget = 1.0f; // Coarsening must project within `budget * ratio`, while refining triggers at `budget` — the gap is // the dead band, and 1.0 disables it. @@ -162,23 +167,27 @@ inline constexpr float kShadowLodPixelBudget = 1.0f; // threshold), which costs triangles — a caster holds finer geometry longer — to buy stability. // // Measured AT THIS BUDGET (chatter depends on which thresholds casters sit near, so a ratio swept -// at a different budget would not reproduce this decision), over ~1100-frame runs of the animated +// at a different budget would not reproduce this decision), over ~600-frame runs of the animated // scene, per 100 frames: // -// ratio 1.0 0.46 transitions 0.00 REVERSALS +// ratio 1.0 0.50 transitions 0.00 REVERSALS // ratio 0.75 0.16 transitions 0.00 REVERSALS -// ratio 0.5 0.15 transitions 0.00 REVERSALS +// ratio 0.5 0.16 transitions 0.00 REVERSALS // -// (Re-measured after SH-04. The transition rates moved — 0.27/0.09/0.09 before — but they are 3, 1 -// and 1 raw events over ~650 frames, so the rates are dominated by counting noise and by where the -// wall-clock-driven animation happened to be. The REVERSAL column, the only one that can justify a -// ratio, is still zero at every ratio, which is the conclusion that matters.) +// RE-MEASURED 2026-08-01, after the glTF loader stopped dropping lights on animated nodes. Until +// then `ShadowLodMotionDemo`'s authored sun was silently replaced by the engine's fallback +// directional, so the scene's sun-swing animation did nothing and every dead-band figure taken on +// it described a STATIC sun. The rates barely moved (0.46/0.16/0.15 before) because they are 3, 1 +// and 1 raw events over ~600 frames — dominated by counting noise — and the REVERSAL column, the +// only one that can justify a ratio, is still zero at every ratio. // // Chatter is a reversal that undoes a RECENT transition (within kReversalWindowCommits) — a caster // oscillating across a threshold. Counting every return would have scored the scene's periodic // animation as instability: an earlier, time-blind reversal count reported 0.31 per 100 frames, -// which was a caster legitimately walking L1 → L2 and back as the sun swung. With the window -// applied there is no chatter at all, at any ratio. +// which was a caster legitimately walking L1 -> L2 and back as the MOVING CASTER crossed a +// threshold. (That was first written up as the sun swinging, which it cannot have been: under the +// loader defect above the sun never moved. The correction does not change the finding.) With the +// window applied there is no chatter at all, at any ratio. // // Plain transitions likewise include ordinary motion that no dead band can or should remove, so // only the reversal column can justify a ratio — and it is zero. diff --git a/src/core/gltf_loader_nodes.cpp b/src/core/gltf_loader_nodes.cpp index aa33192e..946f782d 100644 --- a/src/core/gltf_loader_nodes.cpp +++ b/src/core/gltf_loader_nodes.cpp @@ -1,4 +1,5 @@ #include +#include #include #include @@ -67,26 +68,33 @@ Light::Type toLightType(fastgltf::LightType t) noexcept } } -// KHR_lights_punctual: attach a Light component to `node` if the glTF node -// references a light. Only fires when the variant is still Empty -- Light has -// to share the Components variant slot, so a node already carrying a Mesh / -// Animator / Camera skips with a warning. Cone angles only matter for Spot. -void applyLight(const fastgltf::Asset& asset, const fastgltf::Node& gltfNode, Node& node) +// Every direct emplacement goes through this. `materializeNodeComponentLayout` hands out targets +// that are Empty by construction, so a non-Empty one means the layout and the attach sites have +// come apart — two payloads believing they own the same node. Terminal rather than a warning, +// because the failure it replaces was silent: the second emplacement simply destroyed the first, +// and a scene rendered plausibly with a component nobody could find again. +void requireEmptyComponent(const Node& node, std::string_view what) { - if (!gltfNode.lightIndex.has_value()) + if (!std::holds_alternative(node.component())) { - return; + throw std::runtime_error( + std::format("glTF node '{}': cannot attach {} — the target already holds {}", + node.name(), what, componentName(node.component()))); } - if (!std::holds_alternative(node.component())) +} + +// KHR_lights_punctual: attach the node's light to the target the layout chose. Placement is not +// decided here and must not be — see `core/node_component_layout.hpp` for why the previous +// order-dependent version destroyed lights without a word. +void attachLight(const fastgltf::Asset& asset, const fastgltf::Node& gltfNode, Node& target) +{ + if (!gltfNode.lightIndex.has_value()) { - log::warn(log::category::gltf, - "skipping KHR_lights_punctual on node '{}' -- node already has a non-Empty " - "component (mesh/animator)", - node.name()); return; } + requireEmptyComponent(target, "light"); const auto& gl = asset.lights[gltfNode.lightIndex.value()]; - auto& light = node.component().emplace(); + auto& light = target.component().emplace(); light.type(toLightType(gl.type)); light.colour(Colour3{gl.color.x(), gl.color.y(), gl.color.z()}); light.intensity(static_cast(gl.intensity)); @@ -267,33 +275,24 @@ void GltfLoader::GltfSceneBuilder::applyPhysicsConfig(std::size_t nodeIndex, node.physicsColliderHandle(context_.physics.createCollider(bodyHandle, colliderDesc)); } -Node& GltfLoader::GltfSceneBuilder::attachCamera(Node& node) +// The camera goes on the node the LAYOUT chose — this function no longer inspects the variant to +// decide for itself. That inspection is exactly how placement drifted from the rule: each attach +// site read the current state and reached its own conclusion, so the order of the calls became the +// real policy. +Node& GltfLoader::GltfSceneBuilder::attachCamera(Node& target) { - auto configureCamera = [](Camera& camera) - { - camera.localPosition({0.0f, 0.0f, 0.0f}); - camera.localYaw(-pi / 2.0f); - camera.localPitch(0.0f); - }; - - Node* cameraNode = &node; - if (std::holds_alternative(node.component())) - { - configureCamera(node.component().emplace()); - } - else - { - auto child = std::make_unique(node.name() + "_Camera"); - cameraNode = &node.addChild(std::move(child)); - configureCamera(cameraNode->component().emplace()); - } + requireEmptyComponent(target, "camera"); + Camera& camera = target.component().emplace(); + camera.localPosition({0.0f, 0.0f, 0.0f}); + camera.localYaw(-pi / 2.0f); + camera.localPitch(0.0f); if (context_.activeCamera == nullptr) { - context_.activeCamera = cameraNode; + context_.activeCamera = ⌖ } - return *cameraNode; + return target; } Mesh& GltfLoader::GltfSceneBuilder::attachMeshToNode(std::size_t nodeIndex, std::size_t meshIndex, @@ -380,12 +379,6 @@ void GltfLoader::GltfSceneBuilder::configureAnimatedNode(std::size_t nodeIndex, applyTRS(gltfNode, node); } - // KHR_lights_punctual on this node: only attaches if the node won't - // otherwise be holding a Mesh / Animator (helper guards via variant - // alternative check). Animated nodes lose the light with a warning -- - // rare in practice and worth documenting once, not redesigning for. - applyLight(asset, gltfNode, node); - // Load each glTF animation as a separate Animation object for this node std::vector> nodeAnimations; for (std::size_t ai = 0; ai < asset.animations.size(); ++ai) @@ -412,41 +405,35 @@ void GltfLoader::GltfSceneBuilder::configureAnimatedNode(std::size_t nodeIndex, nodeAnimations.emplace_back(ai, &la); } - if (hasTransformAnim) + // ONE decision, taken before anything is attached: which node holds what. Every attach below + // consumes a target rather than inspecting the current variant, so no reordering of these calls + // can change placement — the failure mode that silently destroyed lights. + const NodeComponentLayout layout = + planNodeComponents(hasTransformAnim, gltfNode.meshIndex.has_value(), + gltfNode.lightIndex.has_value(), gltfNode.cameraIndex.has_value()); + const std::string meshChildName = + gltfNode.meshIndex.has_value() && !asset.meshes[gltfNode.meshIndex.value()].name.empty() + ? std::string(asset.meshes[gltfNode.meshIndex.value()].name) + : std::string{}; + const NodeComponentTargets targets = + materializeNodeComponentLayout(node, layout, meshChildName); + + if (layout.primary == NodeComponentLayout::Primary::Animator) { - // Node gets an Animator for transform; mesh goes on a child node + requireEmptyComponent(node, "animator"); node.component().emplace(); auto& animator = std::get(node.component()); for (const auto& [animId, anim] : nodeAnimations) { animator.addAnimation(animId, anim); } - - if (gltfNode.meshIndex.has_value()) - { - const auto& gltfMesh = asset.meshes[gltfNode.meshIndex.value()]; - std::string meshName = gltfMesh.name.empty() ? std::string(gltfNode.name) + "_Mesh" - : std::string(gltfMesh.name); - auto meshNode = std::make_unique(std::move(meshName)); - auto& meshRef = node.addChild(std::move(meshNode)); - Mesh& mesh = attachMeshToNode(nodeIndex, gltfNode.meshIndex.value(), meshRef, node); - - if (hasWeightAnim) - { - for (const auto& [animId, anim] : nodeAnimations) - { - mesh.addMorphAnimation(animId, anim); - } - mesh.initialMorphWeights(std::vector(numMorphTargets, 0.0f)); - } - } } - else if (gltfNode.meshIndex.has_value()) - { - // Only weight animation -- mesh goes directly on this node - const auto& gltfMesh = asset.meshes[gltfNode.meshIndex.value()]; - Mesh& mesh = attachMeshToNode(nodeIndex, gltfNode.meshIndex.value(), node, node); + if (targets.mesh != nullptr) + { + // Physics stays on the TRANSFORM owner, which is this node whether or not the mesh moved to + // a child of it. + Mesh& mesh = attachMeshToNode(nodeIndex, gltfNode.meshIndex.value(), *targets.mesh, node); if (hasWeightAnim) { for (const auto& [animId, anim] : nodeAnimations) @@ -454,14 +441,22 @@ void GltfLoader::GltfSceneBuilder::configureAnimatedNode(std::size_t nodeIndex, mesh.addMorphAnimation(animId, anim); } } + // A transform-animated node starts its morph weights at zero (the animation drives them); + // a weight-only-animated node keeps the glTF's authored weights. + mesh.initialMorphWeights( + hasTransformAnim + ? std::vector(numMorphTargets, 0.0f) + : initialMorphWeights(asset.meshes[gltfNode.meshIndex.value()], numMorphTargets)); + } - // Apply initial weights from glTF mesh - mesh.initialMorphWeights(initialMorphWeights(gltfMesh, numMorphTargets)); + if (targets.light != nullptr) + { + attachLight(asset, gltfNode, *targets.light); } - if (gltfNode.cameraIndex.has_value()) + if (targets.camera != nullptr) { - attachCamera(node); + attachCamera(*targets.camera); } for (auto childIndex : gltfNode.children) @@ -489,12 +484,21 @@ void GltfLoader::GltfSceneBuilder::loadNode(std::size_t nodeIndex, Node& node) validatePhysicsTarget(nodeIndex, gltfNode); applyTRS(gltfNode, node); - applyLight(asset, gltfNode, node); + // Same one decision as the animated path, with no transform animation in play. + const NodeComponentLayout layout = + planNodeComponents(false, gltfNode.meshIndex.has_value(), gltfNode.lightIndex.has_value(), + gltfNode.cameraIndex.has_value()); + const std::string meshChildName = + gltfNode.meshIndex.has_value() && !asset.meshes[gltfNode.meshIndex.value()].name.empty() + ? std::string(asset.meshes[gltfNode.meshIndex.value()].name) + : std::string{}; + const NodeComponentTargets targets = + materializeNodeComponentLayout(node, layout, meshChildName); - if (gltfNode.meshIndex.has_value()) + if (targets.mesh != nullptr) { const auto& gltfMesh = asset.meshes[gltfNode.meshIndex.value()]; - Mesh& mesh = attachMeshToNode(nodeIndex, gltfNode.meshIndex.value(), node, node); + Mesh& mesh = attachMeshToNode(nodeIndex, gltfNode.meshIndex.value(), *targets.mesh, node); // Static meshes with morph targets still honour mesh.weights (e.g. // MorphPrimitivesTest). Without this, weights stay at zero and the @@ -506,9 +510,14 @@ void GltfLoader::GltfSceneBuilder::loadNode(std::size_t nodeIndex, Node& node) } } - if (gltfNode.cameraIndex.has_value()) + if (targets.light != nullptr) + { + attachLight(asset, gltfNode, *targets.light); + } + + if (targets.camera != nullptr) { - attachCamera(node); + attachCamera(*targets.camera); } for (auto childIndex : gltfNode.children) diff --git a/src/core/node_component_layout.cpp b/src/core/node_component_layout.cpp new file mode 100644 index 00000000..30c587e8 --- /dev/null +++ b/src/core/node_component_layout.cpp @@ -0,0 +1,90 @@ +#include + +#include +#include + +#include + +namespace fire_engine +{ + +NodeComponentLayout planNodeComponents(bool hasTransformAnim, bool hasMesh, bool hasLight, + bool hasCamera) noexcept +{ + NodeComponentLayout layout{}; + + if (hasTransformAnim) + { + // The animator cannot move to a child: the child would then inherit an unanimated parent + // transform and everything else on the node would stop following the animation. + layout.primary = NodeComponentLayout::Primary::Animator; + layout.meshOnChild = hasMesh; + layout.lightOnChild = hasLight; + layout.cameraOnChild = hasCamera; + return layout; + } + + if (hasMesh) + { + layout.primary = NodeComponentLayout::Primary::Mesh; + layout.lightOnChild = hasLight; + layout.cameraOnChild = hasCamera; + return layout; + } + + if (hasLight) + { + layout.primary = NodeComponentLayout::Primary::Light; + layout.cameraOnChild = hasCamera; + return layout; + } + + // A camera alone sits on the node; with anything else it took a child already, before this rule + // existed. An empty layout — a pure transform node — is the remaining case. + layout.primary = + hasCamera ? NodeComponentLayout::Primary::Camera : NodeComponentLayout::Primary::Empty; + return layout; +} + +NodeComponentTargets materializeNodeComponentLayout(Node& node, const NodeComponentLayout& layout, + std::string_view meshChildName) +{ + // Children are created with the default (identity) transform and never given one: the glTF + // node's own transform stays on the parent, which is what makes an animated parent drive its + // mesh, light and camera together instead of one of them. + const auto addChild = [&node](std::string name) -> Node* + { return &node.addChild(std::make_unique(std::move(name))); }; + + NodeComponentTargets targets{}; + if (layout.primary == NodeComponentLayout::Primary::Mesh) + { + targets.mesh = &node; + } + else if (layout.meshOnChild) + { + targets.mesh = + addChild(meshChildName.empty() ? node.name() + "_Mesh" : std::string(meshChildName)); + } + + if (layout.primary == NodeComponentLayout::Primary::Light) + { + targets.light = &node; + } + else if (layout.lightOnChild) + { + targets.light = addChild(node.name() + "_Light"); + } + + if (layout.primary == NodeComponentLayout::Primary::Camera) + { + targets.camera = &node; + } + else if (layout.cameraOnChild) + { + targets.camera = addChild(node.name() + "_Camera"); + } + + return targets; +} + +} // namespace fire_engine diff --git a/tests/assets/node_component_decomposition.gltf b/tests/assets/node_component_decomposition.gltf new file mode 100644 index 00000000..319c197e --- /dev/null +++ b/tests/assets/node_component_decomposition.gltf @@ -0,0 +1,255 @@ +{ + "asset": { + "version": "2.0", + "generator": "fire_engine tests" + }, + "extensionsUsed": [ + "KHR_lights_punctual" + ], + "extensions": { + "KHR_lights_punctual": { + "lights": [ + { + "type": "directional", + "color": [ + 1.0, + 0.9, + 0.8 + ], + "intensity": 2.0, + "name": "AnimatedSun" + }, + { + "type": "point", + "color": [ + 0.2, + 0.4, + 1.0 + ], + "intensity": 3.0, + "range": 12.0, + "name": "StaticLamp" + }, + { + "type": "spot", + "color": [ + 1.0, + 1.0, + 1.0 + ], + "intensity": 1.0, + "name": "CameraLamp", + "spot": { + "innerConeAngle": 0.2, + "outerConeAngle": 0.5 + } + }, + { + "type": "point", + "color": [ + 1.0, + 0.0, + 0.0 + ], + "intensity": 1.0, + "name": "AnimatedMeshLamp" + } + ] + } + }, + "scene": 0, + "scenes": [ + { + "nodes": [ + 0, + 1, + 2, + 3, + 4 + ] + } + ], + "nodes": [ + { + "name": "AnimatedLight", + "extensions": { + "KHR_lights_punctual": { + "light": 0 + } + }, + "rotation": [ + 0.0, + 0.0, + 0.0, + 1.0 + ] + }, + { + "name": "StaticMeshLight", + "mesh": 0, + "translation": [ + 3.0, + 0.0, + 0.0 + ], + "extensions": { + "KHR_lights_punctual": { + "light": 1 + } + } + }, + { + "name": "CameraLight", + "camera": 0, + "translation": [ + 0.0, + 4.0, + 0.0 + ], + "extensions": { + "KHR_lights_punctual": { + "light": 2 + } + } + }, + { + "name": "AnimatedMeshLight", + "mesh": 0, + "translation": [ + -3.0, + 0.0, + 0.0 + ], + "extensions": { + "KHR_lights_punctual": { + "light": 3 + } + } + }, + { + "name": "AnimatedCamera", + "camera": 0, + "translation": [ + 0.0, + 0.0, + 6.0 + ] + } + ], + "cameras": [ + { + "type": "perspective", + "perspective": { + "yfov": 0.8, + "znear": 0.1, + "zfar": 100.0 + } + } + ], + "meshes": [ + { + "primitives": [ + { + "attributes": { + "POSITION": 0 + } + } + ] + } + ], + "animations": [ + { + "name": "Spin", + "channels": [ + { + "sampler": 0, + "target": { + "node": 0, + "path": "rotation" + } + }, + { + "sampler": 0, + "target": { + "node": 3, + "path": "rotation" + } + }, + { + "sampler": 0, + "target": { + "node": 4, + "path": "rotation" + } + } + ], + "samplers": [ + { + "input": 1, + "output": 2, + "interpolation": "LINEAR" + } + ] + } + ], + "buffers": [ + { + "byteLength": 76, + "uri": "data:application/octet-stream;base64,AAAAAAAAAAAAAAAAAACAPwAAAAAAAAAAAAAAAAAAgD8AAAAAAAAAAAAAgD8AAAAAAAAAAAAAAAAAAIA/AAAAAPQENT8AAAAA9AQ1Pw==" + } + ], + "bufferViews": [ + { + "buffer": 0, + "byteOffset": 0, + "byteLength": 36, + "target": 34962 + }, + { + "buffer": 0, + "byteOffset": 36, + "byteLength": 8 + }, + { + "buffer": 0, + "byteOffset": 44, + "byteLength": 32 + } + ], + "accessors": [ + { + "bufferView": 0, + "componentType": 5126, + "count": 3, + "type": "VEC3", + "min": [ + 0.0, + 0.0, + 0.0 + ], + "max": [ + 1.0, + 1.0, + 0.0 + ] + }, + { + "bufferView": 1, + "componentType": 5126, + "count": 2, + "type": "SCALAR", + "min": [ + 0.0 + ], + "max": [ + 1.0 + ] + }, + { + "bufferView": 2, + "componentType": 5126, + "count": 2, + "type": "VEC4" + } + ] +} diff --git a/tests/core/test_gltf_node_decomposition.cpp b/tests/core/test_gltf_node_decomposition.cpp new file mode 100644 index 00000000..b53a28e2 --- /dev/null +++ b/tests/core/test_gltf_node_decomposition.cpp @@ -0,0 +1,228 @@ +#include +#include + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using fire_engine::Assets; +using fire_engine::Device; +using fire_engine::GltfLoader; +using fire_engine::InputState; +using fire_engine::Lighting; +using fire_engine::Node; +using fire_engine::PhysicsWorld; +using fire_engine::Resources; +using fire_engine::SceneGraph; + +// ============================================================================================ +// glTF node decomposition, through the PRODUCTION loader ([.][gpu], local only — `loadScene` +// needs a real `Resources`, so this cannot run on a CI container with no ICD; the rule itself is +// pinned headlessly in `test_node_component_layout.cpp`). +// +// The fixture (`tests/assets/node_component_decomposition.gltf`) authors one node per case a +// single-component variant has to resolve. THREE of them were broken: animated + light, static +// mesh + light, and animated mesh + light, where the light was attached while the node was Empty +// and then overwritten by `emplace()` / `emplace()` with no warning, because the +// guard had already passed. The other two — camera + light, and animated camera — already worked; +// they are here as regression guards, since all five now share one placement rule and a change to +// it would break them together. +// ============================================================================================ + +namespace +{ + +// Depth-first search for a named node, so a test can assert on the SHAPE of the decomposition +// rather than on load order. +[[nodiscard]] const Node* findNode(const Node& node, std::string_view name) +{ + if (node.name() == name) + { + return &node; + } + for (const auto& child : node.children()) + { + if (const Node* found = findNode(*child, name)) + { + return found; + } + } + return nullptr; +} + +[[nodiscard]] const Node* findNode(const SceneGraph& scene, std::string_view name) +{ + for (const auto& root : scene.nodes()) + { + if (const Node* found = findNode(*root, name)) + { + return found; + } + } + return nullptr; +} + +[[nodiscard]] std::size_t countLights(const Node& node) +{ + std::size_t count = node.componentAs() != nullptr ? 1u : 0u; + for (const auto& child : node.children()) + { + count += countLights(*child); + } + return count; +} + +} // namespace + +TEST_CASE("GltfNodeDecomposition.EveryAuthoredComponentSurvivesTheLoad", "[.][gpu][GltfLoader]") +{ + Device device = Device::headlessCompute(); + Resources resources(device); + SceneGraph scene; + Assets assets; + PhysicsWorld physics; + GltfLoader::loadScene(std::string(FIRE_ENGINE_BUILD_ASSET_DIR) + + "/test_assets/node_component_decomposition.gltf", + scene, resources, assets, physics); + + SECTION("an animated light survives as a child of its animator") + { + const Node* animated = findNode(scene, "AnimatedLight"); + REQUIRE(animated != nullptr); + CHECK(animated->componentAs() != nullptr); + const Node* light = findNode(scene, "AnimatedLight_Light"); + REQUIRE(light != nullptr); + REQUIRE(light->componentAs() != nullptr); + CHECK(light->componentAs()->type() == + fire_engine::Light::Type::Directional); + // Exactly one, on the child: a rule that also left one on the parent would double the sun. + CHECK(countLights(*animated) == 1u); + } + SECTION("a static mesh and light both survive") + { + const Node* node = findNode(scene, "StaticMeshLight"); + REQUIRE(node != nullptr); + CHECK(node->componentAs() != nullptr); + const Node* light = findNode(scene, "StaticMeshLight_Light"); + REQUIRE(light != nullptr); + REQUIRE(light->componentAs() != nullptr); + CHECK(light->componentAs()->type() == fire_engine::Light::Type::Point); + } + SECTION("a camera and light both survive") + { + const Node* node = findNode(scene, "CameraLight"); + REQUIRE(node != nullptr); + // The light claims the node, the camera takes the child — the camera path already behaved + // this way, and the rule now states it. + CHECK(node->componentAs() != nullptr); + const Node* camera = findNode(scene, "CameraLight_Camera"); + REQUIRE(camera != nullptr); + CHECK(camera->componentAs() != nullptr); + } + SECTION("an animated mesh and light become siblings under one animator") + { + const Node* node = findNode(scene, "AnimatedMeshLight"); + REQUIRE(node != nullptr); + CHECK(node->componentAs() != nullptr); + CHECK(countLights(*node) == 1u); + const Node* light = findNode(scene, "AnimatedMeshLight_Light"); + REQUIRE(light != nullptr); + CHECK(light->componentAs() != nullptr); + // The mesh is somewhere under the same animator, not displaced by the light. + bool meshFound = false; + for (const auto& child : node->children()) + { + meshFound = meshFound || child->componentAs() != nullptr; + } + CHECK(meshFound); + } + SECTION("an animated camera still works, and still moves") + { + // Not a fix — a regression guard. This path already created a child; it must keep doing so + // now that the light shares the same rule. + const Node* node = findNode(scene, "AnimatedCamera"); + REQUIRE(node != nullptr); + CHECK(node->componentAs() != nullptr); + const Node* camera = findNode(scene, "AnimatedCamera_Camera"); + REQUIRE(camera != nullptr); + CHECK(camera->componentAs() != nullptr); + + // Existing is not enough: a camera parked on an identity child of an animator would satisfy + // every structural check above while sitting perfectly still. Advance the clock and require + // the child's WORLD pose to follow its parent's animation. + InputState input; + input.time(0.0); + scene.update(input); + const fire_engine::Mat4 atRest = camera->composedWorld(); + input.time(0.5); + scene.update(input); + const fire_engine::Mat4 later = camera->composedWorld(); + + // Compare the forward axis: the fixture rotates, so orientation is where the motion shows. + const fire_engine::Vec3 restForward{-atRest[0, 2], -atRest[1, 2], -atRest[2, 2]}; + const fire_engine::Vec3 laterForward{-later[0, 2], -later[1, 2], -later[2, 2]}; + CHECK(fire_engine::Vec3::dotProduct(restForward, laterForward) < 0.99f); + } + SECTION("every authored light is gathered exactly once, with a stable identity") + { + InputState input; + scene.update(input); + const std::vector first = scene.gatherLights(); + // Four authored lights, four gathered: none lost, none duplicated. + CHECK(first.size() == 4u); + CHECK(scene.hasDirectionalLight()); + + scene.update(input); + const std::vector second = scene.gatherLights(); + REQUIRE(second.size() == first.size()); + for (std::size_t i = 0; i < first.size(); ++i) + { + // Shadow state is keyed on this: an identity that moved between frames would reset a + // caster's hysteresis every frame. + CHECK(second[i].nodeId == first[i].nodeId); + } + } + SECTION("the animated light's direction actually follows its animation") + { + InputState input; + input.time(0.0); + scene.update(input); + const auto atRest = scene.gatherLights(); + // The animator samples an ABSOLUTE clock (`InputState::time`), not a per-frame delta, so + // advancing means moving that clock. Half a second lands mid-way through the fixture's + // 0->1 s rotation keyframes, where the sampled orientation is unambiguously different. + input.time(0.5); + scene.update(input); + const auto later = scene.gatherLights(); + REQUIRE(atRest.size() == later.size()); + + const Lighting* restSun = nullptr; + const Lighting* laterSun = nullptr; + for (std::size_t i = 0; i < atRest.size(); ++i) + { + if (atRest[i].type == 0) + { + restSun = &atRest[i]; + laterSun = &later[i]; + } + } + REQUIRE(restSun != nullptr); + // The whole point of keeping the light under the animator: the animation reaches it. + const float dot = + fire_engine::Vec3::dotProduct(restSun->worldDirection, laterSun->worldDirection); + CHECK(dot < 0.99f); + } +} diff --git a/tests/core/test_node_component_layout.cpp b/tests/core/test_node_component_layout.cpp new file mode 100644 index 00000000..8bc75a48 --- /dev/null +++ b/tests/core/test_node_component_layout.cpp @@ -0,0 +1,209 @@ +#include + +#include +#include +#include +#include +#include + +using fire_engine::materializeNodeComponentLayout; +using fire_engine::Node; +using fire_engine::NodeComponentLayout; +using fire_engine::NodeComponentTargets; +using fire_engine::planNodeComponents; +using Primary = fire_engine::NodeComponentLayout::Primary; + +// The rule that replaces "whatever the attach order happened to be". Exhaustive over all sixteen +// combinations, because the defect it fixes was precisely a combination nobody had enumerated: a +// glTF node carrying BOTH a light and something that claims the single component slot silently lost +// the light, and `ShadowLodMotionDemo` then rendered every measurement under a fallback sun. +TEST_CASE("NodeComponentLayout.EveryCombinationPlacesEveryPieceOfContent", "[NodeComponentLayout]") +{ + for (int mask = 0; mask < 16; ++mask) + { + const bool anim = (mask & 1) != 0; + const bool mesh = (mask & 2) != 0; + const bool light = (mask & 4) != 0; + const bool camera = (mask & 8) != 0; + INFO("anim=" << anim << " mesh=" << mesh << " light=" << light << " camera=" << camera); + + const NodeComponentLayout layout = planNodeComponents(anim, mesh, light, camera); + + // NOTHING IS LOST. Each piece of content the glTF node declared ends up either on the node + // or on a child of it — the property whose absence was the bug. + const bool meshPlaced = layout.primary == Primary::Mesh || layout.meshOnChild; + const bool lightPlaced = layout.primary == Primary::Light || layout.lightOnChild; + const bool cameraPlaced = layout.primary == Primary::Camera || layout.cameraOnChild; + CHECK(meshPlaced == mesh); + CHECK(lightPlaced == light); + CHECK(cameraPlaced == camera); + // And an animated node always owns its Animator, since a child cannot animate its parent. + CHECK((layout.primary == Primary::Animator) == anim); + + // Nothing is placed twice, and nothing is invented: the primary slot is never also a child. + const bool meshTwice = layout.primary == Primary::Mesh && layout.meshOnChild; + const bool lightTwice = layout.primary == Primary::Light && layout.lightOnChild; + const bool cameraTwice = layout.primary == Primary::Camera && layout.cameraOnChild; + CHECK_FALSE(meshTwice); + CHECK_FALSE(lightTwice); + CHECK_FALSE(cameraTwice); + } +} + +TEST_CASE("NodeComponentLayout.PrecedenceIsByWhatCannotMove", "[NodeComponentLayout]") +{ + SECTION("an animated light rides a child of its animator") + { + // The ShadowLodMotionDemo case exactly: a node with a rotation channel and a directional + // light. Before the rule, the light was attached and then overwritten by the Animator. + const auto layout = planNodeComponents(true, false, true, false); + CHECK(layout.primary == Primary::Animator); + CHECK(layout.lightOnChild); + CHECK_FALSE(layout.meshOnChild); + } + SECTION("a static mesh and light both survive") + { + // The other half of the same defect, with no animation involved: `emplace` used to + // overwrite the light attached moments earlier. + const auto layout = planNodeComponents(false, true, true, false); + CHECK(layout.primary == Primary::Mesh); + CHECK(layout.lightOnChild); + } + SECTION("an animated mesh and light become two children of one animator") + { + const auto layout = planNodeComponents(true, true, true, false); + CHECK(layout.primary == Primary::Animator); + CHECK(layout.meshOnChild); + CHECK(layout.lightOnChild); + } + SECTION("a camera yields the node to anything else") + { + CHECK(planNodeComponents(false, false, false, true).primary == Primary::Camera); + CHECK_FALSE(planNodeComponents(false, false, false, true).cameraOnChild); + // Light + camera: the light takes the node, the camera takes a child — the behaviour the + // camera path already had, now stated rather than implied by call order. + const auto withLight = planNodeComponents(false, false, true, true); + CHECK(withLight.primary == Primary::Light); + CHECK(withLight.cameraOnChild); + } + SECTION("weight-only animation is not transform animation") + { + // Morph weights are driven through the Mesh component, so a weight-animated node needs no + // Animator and lays out as a plain mesh node. Passing `hasTransformAnim = false` is the + // caller's job; this pins what the rule does with it. + const auto layout = planNodeComponents(false, true, false, false); + CHECK(layout.primary == Primary::Mesh); + CHECK_FALSE(layout.meshOnChild); + } + SECTION("a node carrying nothing stays empty") + { + const auto layout = planNodeComponents(false, false, false, false); + CHECK(layout.primary == Primary::Empty); + CHECK_FALSE(layout.meshOnChild); + CHECK_FALSE(layout.lightOnChild); + CHECK_FALSE(layout.cameraOnChild); + } +} + +// The plan is only worth anything if PRODUCTION applies it, so the materialisation that the loader +// calls is exercised here on real nodes — headlessly, which is the point: the `[.][gpu]` loader +// fixture confirms the same shapes end to end, but CI can only run this one, and this one covers +// the whole topology rather than a single flag. +TEST_CASE("NodeComponentLayout.MaterialisationPlacesEveryPayloadOnAnEmptyTarget", + "[NodeComponentLayout]") +{ + for (int mask = 0; mask < 16; ++mask) + { + const bool anim = (mask & 1) != 0; + const bool mesh = (mask & 2) != 0; + const bool light = (mask & 4) != 0; + const bool camera = (mask & 8) != 0; + INFO("anim=" << anim << " mesh=" << mesh << " light=" << light << " camera=" << camera); + + Node node("Thing"); + const NodeComponentLayout layout = planNodeComponents(anim, mesh, light, camera); + const NodeComponentTargets targets = materializeNodeComponentLayout(node, layout); + + // A target exists for exactly the payloads the glTF node declared — never a fallback. + CHECK((targets.mesh != nullptr) == mesh); + CHECK((targets.light != nullptr) == light); + CHECK((targets.camera != nullptr) == camera); + + // Every target is EMPTY and therefore safe to emplace into. This is the property that + // makes attach order irrelevant, which is the whole fix: two payloads can never be handed + // the same node, so neither can overwrite the other. + const Node* seen[3] = {targets.mesh, targets.light, targets.camera}; + for (int i = 0; i < 3; ++i) + { + if (seen[i] == nullptr) + { + continue; + } + CHECK(seen[i]->componentAs() != nullptr); + for (int j = i + 1; j < 3; ++j) + { + const bool sameTarget = seen[j] != nullptr && seen[i] == seen[j]; + CHECK_FALSE(sameTarget); + } + } + + // Children are created only where the layout called for one, and each is IDENTITY: the + // glTF transform stays on the parent, so an animated parent drives all of them together. + const std::size_t expectedChildren = static_cast(layout.meshOnChild) + + static_cast(layout.lightOnChild) + + static_cast(layout.cameraOnChild); + CHECK(node.children().size() == expectedChildren); + for (const auto& child : node.children()) + { + CHECK(child->transform().position() == fire_engine::Vec3{}); + CHECK(child->transform().rotation() == fire_engine::Quaternion{}); + } + } +} + +TEST_CASE("NodeComponentLayout.MaterialisationNamesAndParentsTheChildren", "[NodeComponentLayout]") +{ + Node node("Lamp"); + const NodeComponentLayout layout = planNodeComponents(true, true, true, true); + const NodeComponentTargets targets = materializeNodeComponentLayout(node, layout); + + // The animated node keeps itself for the Animator; everything else hangs off it. + CHECK(targets.mesh != &node); + CHECK(targets.light != &node); + CHECK(targets.camera != &node); + REQUIRE(node.children().size() == 3u); + CHECK(targets.mesh->name() == "Lamp_Mesh"); + CHECK(targets.light->name() == "Lamp_Light"); + CHECK(targets.camera->name() == "Lamp_Camera"); + + // glTF meshes carry their own names and the loader prefers them, so the mesh child can be named + // by the caller; the light and camera children never are. + Node named("Lamp"); + const NodeComponentTargets namedTargets = + materializeNodeComponentLayout(named, layout, "TeapotMesh"); + CHECK(namedTargets.mesh->name() == "TeapotMesh"); + CHECK(namedTargets.light->name() == "Lamp_Light"); +} + +TEST_CASE("NodeComponentLayout.MaterialisationKeepsSoleContentOnTheNode", "[NodeComponentLayout]") +{ + // No children when nothing competes: a static mesh node stays one node, which is what every + // existing scene expects. + Node meshOnly("Rock"); + const NodeComponentTargets mesh = + materializeNodeComponentLayout(meshOnly, planNodeComponents(false, true, false, false)); + CHECK(mesh.mesh == &meshOnly); + CHECK(meshOnly.children().empty()); + + Node lightOnly("Sun"); + const NodeComponentTargets sun = + materializeNodeComponentLayout(lightOnly, planNodeComponents(false, false, true, false)); + CHECK(sun.light == &lightOnly); + CHECK(lightOnly.children().empty()); + + Node cameraOnly("Eye"); + const NodeComponentTargets eye = + materializeNodeComponentLayout(cameraOnly, planNodeComponents(false, false, false, true)); + CHECK(eye.camera == &cameraOnly); + CHECK(cameraOnly.children().empty()); +} diff --git a/tests/scene/test_light.cpp b/tests/scene/test_light.cpp index 33086e39..3031c18b 100644 --- a/tests/scene/test_light.cpp +++ b/tests/scene/test_light.cpp @@ -173,11 +173,14 @@ TEST_CASE("LightGather.ScaledNodeStillEmitsUnitForward", "[LightGather]") // --------------------------------------------------------------------------- #include +#include #include #include +using fire_engine::Animator; using fire_engine::InputState; using fire_engine::Node; +using fire_engine::Quaternion; using fire_engine::SceneGraph; TEST_CASE("GatherLights.EmptySceneReturnsNoLights", "[GatherLights]") @@ -325,3 +328,92 @@ TEST_CASE("HasDirectionalLight.FindsDirectionalNestedAsChild", "[HasDirectionalL scene.addNode(std::move(root)); CHECK(scene.hasDirectionalLight()); } + +// --------------------------------------------------------------------------- +// The decomposition the glTF loader now produces for an animated light: an +// Animator on the transform-owning node, the Light on an identity-transform +// child. These pin the SCENE-side consequences of that layout, which is where +// the original defect actually bit — the light vanished, `hasDirectionalLight` +// returned false, and FireEngine seeded its fallback sun over the top. +// --------------------------------------------------------------------------- + +TEST_CASE("AnimatedLightLayout.LightOnChildOfAnimatorSuppressesTheFallbackSun", "[Light]") +{ + SceneGraph scene; + auto animated = std::make_unique("Sun"); + animated->component().emplace(); + auto lightNode = std::make_unique("Sun_Light"); + lightNode->component().emplace(); // default type = Directional + animated->addChild(std::move(lightNode)); + scene.addNode(std::move(animated)); + + // What FireEngine consults before seeding its own directional. False here is what put a + // fallback sun into ShadowLodMotionDemo and silently replaced the authored one. + CHECK(scene.hasDirectionalLight()); + + InputState input; + scene.update(input); + const auto lights = scene.gatherLights(); + // EXACTLY one: a layout that placed the light on both the node and a child would light the + // scene twice and look merely "a bit bright". + REQUIRE(lights.size() == 1u); + CHECK(lights[0].type == 0); // directional +} + +TEST_CASE("AnimatedLightLayout.ChildLightFollowsItsAnimatedParentTransform", "[Light]") +{ + SceneGraph scene; + auto animated = std::make_unique("Sun"); + animated->component().emplace(); + auto lightNode = std::make_unique("Sun_Light"); + lightNode->component().emplace(); + Node& lightRef = animated->addChild(std::move(lightNode)); + Node& parentRef = scene.addNode(std::move(animated)); + + // The child is identity: every bit of its world orientation comes from the parent, which is + // exactly why the light can be moved off the animated node without losing the animation. + CHECK(lightRef.transform().rotation() == Quaternion{}); + + InputState input; + parentRef.transform().rotation( + Quaternion::fromVectors({0.0f, 0.0f, -1.0f}, {1.0f, 0.0f, 0.0f})); + scene.update(input); + const auto pointedX = scene.gatherLights(); + REQUIRE(pointedX.size() == 1u); + CHECK(pointedX[0].worldDirection.x() == Catch::Approx(1.0f).margin(1e-5)); + + parentRef.transform().rotation( + Quaternion::fromVectors({0.0f, 0.0f, -1.0f}, {0.0f, -1.0f, 0.0f})); + scene.update(input); + const auto pointedDown = scene.gatherLights(); + REQUIRE(pointedDown.size() == 1u); + CHECK(pointedDown[0].worldDirection.y() == Catch::Approx(-1.0f).margin(1e-5)); + // The identity that keys shadow state must not move when the direction does. + CHECK(pointedDown[0].nodeId == pointedX[0].nodeId); +} + +TEST_CASE("AnimatedLightLayout.SiblingComponentsDoNotDisplaceTheLight", "[Light]") +{ + // One Animator parent with two identity children. The defect was a SECOND component displacing + // the light in a single-component variant; here the sibling exists and the light survives it. + // (The mesh + light pairing is pinned on the rule itself in + // `tests/core/test_node_component_layout.cpp`, where no GPU-backed Mesh is needed.) + SceneGraph scene; + auto animated = std::make_unique("Lamp"); + animated->component().emplace(); + auto cameraNode = std::make_unique("Lamp_Camera"); + cameraNode->component().emplace(); + animated->addChild(std::move(cameraNode)); + auto lightNode = std::make_unique("Lamp_Light"); + auto& light = lightNode->component().emplace(); + light.type(Light::Type::Point); + animated->addChild(std::move(lightNode)); + scene.addNode(std::move(animated)); + + InputState input; + scene.update(input); + const auto lights = scene.gatherLights(); + REQUIRE(lights.size() == 1u); + CHECK(lights[0].type == 1); // point + CHECK_FALSE(scene.hasDirectionalLight()); +}