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()); +}