Skip to content

Commit 7d668ed

Browse files
schellm0ppers
andauthored
Collect scene nodes recursively (#224)
* ✨ collect scene nodes recursively * Rename nodes_in_scene to root_nodes_in_scene, add recursive_nodes_in_scene Split the scene node query API into two explicit methods: - root_nodes_in_scene: returns only the top-level nodes directly referenced by the scene (the original behavior) - recursive_nodes_in_scene: returns all nodes including descendants in depth-first order This fixes a bug where animations targeting child (parented) nodes were silently skipped because only root nodes were collected. Update call sites: - Animator test now uses recursive_nodes_in_scene for correctness - Example crate simplified by removing manual get_children helper Co-authored-by: Andreas Streichardt <andreas@mop.koeln> * fix cache * use matrix.os instead of runner.os * ensure .cargo exists on CI * ensure .cargo exists on CI * ensure .cargo exists on CI * ensure .cargo exists on CI --------- Co-authored-by: Andreas Streichardt <andreas@mop.koeln>
1 parent 3bf599d commit 7d668ed

File tree

4 files changed

+48
-32
lines changed

4 files changed

+48
-32
lines changed

.github/workflows/push.yaml

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ jobs:
3535
with:
3636
path: ~/.cargo
3737
# THIS KEY MUST MATCH BELOW
38-
key: cargo-cache-${{ env.CARGO_GPU_COMMITSH }}-${{ runner.os }}
38+
key: cargo-cache-${{ env.CARGO_GPU_COMMITSH }}-${{ matrix.os }}
3939
- uses: moonrepo/setup-rust@v1
4040
- run: rustup default stable
4141
- run: rustup update
@@ -65,19 +65,22 @@ jobs:
6565
RUST_LOG: debug
6666
steps:
6767
- uses: actions/checkout@v2
68+
# Run moonrepo/setup-rust BEFORE restoring ~/.cargo cache, because
69+
# moonrepo restores its own ~/.cargo cache which would overwrite
70+
# the cargo-gpu binary installed by the install-cargo-gpu job.
71+
- uses: moonrepo/setup-rust@v1
6872
- uses: actions/cache@v4
6973
with:
7074
path: ~/.cargo
7175
# THIS KEY MUST MATCH ABOVE
72-
key: cargo-cache-${{ env.CARGO_GPU_COMMITSH }}-${{ runner.os }}
76+
key: cargo-cache-${{ env.CARGO_GPU_COMMITSH }}-${{ matrix.os }}
7377
- uses: actions/cache@v4
7478
with:
7579
path: |
7680
${{ needs.install-cargo-gpu.outputs.cachepath-macOS }}
7781
${{ needs.install-cargo-gpu.outputs.cachepath-Linux }}
7882
${{ needs.install-cargo-gpu.outputs.cachepath-Windows }}
79-
key: rust-gpu-cache-0-${{ runner.os }}
80-
- uses: moonrepo/setup-rust@v1
83+
key: rust-gpu-cache-0-${{ matrix.os }}
8184
- run: rustup install nightly
8285
- run: rustup component add --toolchain nightly rustfmt
8386
- run: cargo gpu show commitsh
@@ -96,7 +99,8 @@ jobs:
9699
with:
97100
path: ~/.cargo
98101
# THIS KEY MUST MATCH ABOVE
99-
key: cargo-cache-${{ env.CARGO_GPU_COMMITSH }}-${{ runner.os }}
102+
key: cargo-cache-${{ env.CARGO_GPU_COMMITSH }}-ubuntu-latest
103+
- run: mkdir -p $HOME/.cargo/bin
100104
- uses: moonrepo/setup-rust@v1
101105
- run: rustup install nightly
102106
- run: rustup component add --toolchain nightly rustfmt
@@ -130,8 +134,8 @@ jobs:
130134
- uses: actions/cache@v4
131135
with:
132136
path: ~/.cargo
133-
key: ${{ runner.os }}-test-cargo-${{ hashFiles('**/Cargo.lock') }}
134-
restore-keys: ${{ runner.os }}-cargo-
137+
key: ${{ matrix.os }}-test-cargo-${{ hashFiles('**/Cargo.lock') }}
138+
restore-keys: ${{ matrix.os }}-cargo-
135139

136140
- name: Install linux deps
137141
if: runner.os == 'Linux'
@@ -150,7 +154,7 @@ jobs:
150154
- uses: actions/upload-artifact@v4
151155
if: always()
152156
with:
153-
name: test-output-${{ runner.os }}
157+
name: test-output-${{ matrix.os }}
154158
path: test_output
155159

156160
## WASM tests, commented out until we can get a proper headless browser on CI

crates/example/src/lib.rs

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -286,29 +286,8 @@ impl App {
286286

287287
let scene = doc.default_scene.unwrap_or(0);
288288
log::info!("Displaying scene {scene}");
289-
fn get_children(doc: &GltfDocument, n: usize) -> Vec<usize> {
290-
let mut children = vec![];
291-
if let Some(parent) = doc.nodes.get(n) {
292-
children.extend(parent.children.iter().copied());
293-
let descendants = parent
294-
.children
295-
.iter()
296-
.copied()
297-
.flat_map(|n| get_children(doc, n));
298-
children.extend(descendants);
299-
}
300-
children
301-
}
302289

303-
let nodes = doc.nodes_in_scene(scene).flat_map(|n| {
304-
let mut all_nodes = vec![n];
305-
for child_index in get_children(&doc, n.index) {
306-
if let Some(child_node) = doc.nodes.get(child_index) {
307-
all_nodes.push(child_node);
308-
}
309-
}
310-
all_nodes
311-
});
290+
let nodes = doc.recursive_nodes_in_scene(scene);
312291
log::trace!(" nodes:");
313292
for node in nodes {
314293
let tfrm = Mat4::from(node.global_transform());

crates/renderling/src/gltf.rs

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1237,7 +1237,24 @@ where
12371237
self.primitives.iter().flat_map(|(_, rs)| rs.iter())
12381238
}
12391239

1240-
pub fn nodes_in_scene(&self, scene_index: usize) -> impl Iterator<Item = &GltfNode> {
1240+
fn collect_nodes_recursive<'a>(&'a self, node_index: usize, nodes: &mut Vec<&'a GltfNode>) {
1241+
if let Some(node) = self.nodes.get(node_index) {
1242+
nodes.push(node);
1243+
for child_index in node.children.iter() {
1244+
self.collect_nodes_recursive(*child_index, nodes);
1245+
}
1246+
}
1247+
}
1248+
1249+
/// Returns the root (top-level) nodes in the given scene.
1250+
///
1251+
/// This roughly follows [`gltf::Scene::nodes`](https://docs.rs/gltf/latest/gltf/scene/struct.Scene.html#method.nodes),
1252+
/// returning only the nodes directly referenced by the scene — not
1253+
/// their children.
1254+
///
1255+
/// Use [`recursive_nodes_in_scene`](Self::recursive_nodes_in_scene)
1256+
/// if you need all nodes (including descendants).
1257+
pub fn root_nodes_in_scene(&self, scene_index: usize) -> impl Iterator<Item = &GltfNode> {
12411258
let scene = self.scenes.get(scene_index);
12421259
let mut nodes = vec![];
12431260
if let Some(indices) = scene {
@@ -1250,6 +1267,22 @@ where
12501267
nodes.into_iter()
12511268
}
12521269

1270+
/// Returns all nodes in the given scene, recursively including
1271+
/// children.
1272+
///
1273+
/// Root nodes are visited first, followed by their descendants in
1274+
/// depth-first order.
1275+
pub fn recursive_nodes_in_scene(&self, scene_index: usize) -> impl Iterator<Item = &GltfNode> {
1276+
let scene = self.scenes.get(scene_index);
1277+
let mut nodes = vec![];
1278+
if let Some(indices) = scene {
1279+
for node_index in indices {
1280+
self.collect_nodes_recursive(*node_index, &mut nodes);
1281+
}
1282+
}
1283+
nodes.into_iter()
1284+
}
1285+
12531286
/// Returns the bounding volume of this document, if possible.
12541287
///
12551288
/// This function will return `None` if this document does not contain

crates/renderling/src/gltf/anime.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -787,7 +787,7 @@ mod test {
787787
.unwrap();
788788

789789
let nodes = doc
790-
.nodes_in_scene(doc.default_scene.unwrap_or_default())
790+
.recursive_nodes_in_scene(doc.default_scene.unwrap_or_default())
791791
.collect::<Vec<_>>();
792792

793793
let mut animator = Animator::new(nodes, doc.animations.first().unwrap().clone());

0 commit comments

Comments
 (0)