Node tree rewrite #155

kvark · 2018-01-03T15:57:17Z

Fixes #44

Core of the change is switching relationships from "child->parent" to "parent->first_child->sibling", which gives us the following benefits:

clean children lifetimes, as expected by users
- no need to store all the meshes returned by gltf/obj loaders: perhaps, we can follow-up with a PR that allows querying meshes by name from their parents, so that we don't even return all the meshes directly in the loader API
nice Group::add(&something), like Three-js does
make Scene truly unique and node-less
no need for unique scene IDs
~~- technically, objects can now be present in multiple scenes at once~~
immutable node data upon traversal (!):
- we just compute the world visibility/transform on the fly
- thus, no need for a separate update_transforms step
- uses pretty iterators ;)
- as a downside, not as straightforward to reflect the world info to the user (see TODO)

Has other changes making things nicer:

refactored frame render code
no &mut self on objects
idiomatic hub indexing
safer group/camera/text/audio spawning (internal)
more robust message handling (panic on unprocessed/unexpected messages)
can debug print the Operation

TODO:

check/fix docs
compute world transform/visibility for the user if needed (is it needed?)
provide API to detach from parent
reviews!
profile the cost of recomputing the world transforms

vitvakatu

That's soo cool. Can't wait we will finish it and merge.

vitvakatu · 2018-01-03T16:26:21Z

src/hub.rs

+        for (child_ptr, sibling) in deferred_sibling_updates {
+            let child = &mut self.nodes[&child_ptr];
+            if child.next_sibling.is_some() {
+                error!("Attaching a child that still has an old parent, discarding siblings");


How can we change parent now?

Oh right, I forgot about this :) Now added to TODO list for this PR.
Basically, we can either merge the old siblings with the new ones (like Scene::add does), or require the user to call Group::remove(&item) before adding to another group. Either way, the remove needs to be added.

alteous · 2018-01-03T18:07:06Z

src/hub.rs

+                Operation::AddChild(child_ptr) => match node.sub_node {
+                    SubNode::Group { ref mut first_child } => {
+                        let sibling = mem::replace(first_child, Some(child_ptr.clone()));
+                        deferred_sibling_updates.push((child_ptr, sibling));


Interesting.

borrow checker...

alteous · 2018-01-03T18:07:55Z

src/hub.rs

            SubNode::Visual(_, ref mut gpu_data) => gpu_data.pending = Some(mesh.dynamic.clone()),
            _ => unreachable!(),
        }
    }
+
+    pub(crate) fn walk(&self, base: &Option<NodePointer>) -> TreeWalker {


alteous · 2018-01-03T18:08:41Z

src/node.rs

-    /// The same as `visible`, used internally.
-    pub world_visible: bool,
+    /// Relative to parent transform.
+    pub transform: Transform,
    /// Material in case this `Node` has it.


Was this ever useful?

wait, why is Material here? O_O

Ah, this is user-exposed node, not the internal one.
Yeah, let it be. The whole reflection thing (with sync) I'd consider non-idiomatic, so we shouldn't be too eager to optimize it.

alteous · 2018-01-03T18:09:47Z

src/render/mod.rs

-            };
-            let mx_view = Matrix4::from(w.inverse_transform().unwrap());
+            let node = &hub[&camera];
+            let world_transform = node.transform; //TODO!!!


What's this for?

TODO: compute the world transform for the camera, which is expected here

alteous · 2018-01-03T18:10:28Z

src/scene.rs

 use std::sync::MutexGuard;

-/// Unique identifier for a scene.
-pub type Uid = usize;


alteous

It feels like we're going in the right direction here. The reduced verbosity from all the muts and set_parent calls is a breath of fresh air.

kvark · 2018-01-04T16:09:01Z

Alright, I rebased the code and implemented the missing bits.
Only the profiling is left, plus maybe a few more looks, esp in the new code.
Note: unfortunately, there is a bit of duplication atm between Group reacting to events and Scene implementation. I'm sure we can make it nicer.

kvark · 2018-01-04T18:02:32Z

Here are the profiling results (note: I only used callgrind_annotate, so don't have that much insight at the moment):

descend does show up near the top of the profile
cause likely being large amount of copying (and a bit of stack reallocation). This should be partially solved by future improvements in Rust itself, some possible workarounds from our side, or caching the transforms at least.
group examples with 20K instances runs OK on my U-series CPU, so I think we are fine for now.

Looks like I addressed all the action items from my side. Waiting for more reviews now (@alteous , @vitvakatu ).

kvark · 2018-01-04T18:10:24Z

I believe in the long term our architecture should work like this:

Renderer only walks the tree once, for each node
- figures out what render bins does an object fall into (e.g.: main screen, shadow for light X, off-screen buffer, etc)
- builds the GPU instance blocks for each bin (separately) and appends them to lists to be directly uploaded to GPU constant buffer memory
- writes down GPU commands in the appropriate command buffers

TL;DR: there is no strong reason for Renderer to walk the tree multiple times. We can make rendering much faster on the CPU side, and tree traversal is not (going to be) a bottleneck.

alteous · 2018-01-04T18:37:33Z

examples/group.rs

-const SPEEDS: [f32; 6] = [0.7, -1.0, 1.3, -1.6, 1.9, -2.2];
+struct LevelDesc {
+    color: three::Color,
+    speed: f32,


What is the unit of this? I assume radians/second?

alteous · 2018-01-04T18:41:36Z

src/hub.rs

+        walker
+    }
+
+    pub(crate) fn walk(&self, base: &Option<NodePointer>) -> TreeWalker {


Nit: Perhaps walk_visible?

As in froggy, it's better to use shorter names.

it's for internal use only anyhow. Most of the time we are only concerned about visible nodes

Yes, I remember your strategy 😄

alteous

LGTM.

vitvakatu

We're close

vitvakatu · 2018-01-04T18:40:30Z

Cargo.toml

@@ -28,7 +31,7 @@ cgmath = { version = "0.15", features = ["mint"] }
 derivative = "1.0"
 froggy = "0.4.4"
 genmesh = "0.5"
-gfx = "0.16"
+gfx = "0.16.3"


Because your instancing changes require it

I'm not a fan of strict version rules in Cargo. It will use the latest available version anyway.

I think your position is in the minority within the community then. Latest will be used on fresh checkout, if we don't check in Cargo.lock. But people who already have Cargo.lock will get breakage if you don't write the exact version in the Cargo.toml. Another case is - some crate that depends on you may say gfx = "=0.16.2" and it will try to build but will fail even after cargo update.

TL;DR: Cargo versioning rules are here to stay and be respected

Also, there is a proposal (which I supported) about making cargo publish checking out all the minimal compatible versions (as opposed to latest). Think about it ;)

Okay, you convicted me

vitvakatu · 2018-01-04T18:45:06Z

examples/shapes.rs


-    let mut mline = {
+    // test removal from scene
+    win.scene.remove(&mcyl);


Is it supposed to be here?

yes - it's a small test for object removal. May not be the best place, but better than nothing

vitvakatu · 2018-01-04T18:54:33Z

src/hub.rs

+        walker
+    }
+
+    pub(crate) fn walk(&self, base: &Option<NodePointer>) -> TreeWalker {


As in froggy, it's better to use shorter names.

vitvakatu

Ready to go

vitvakatu · 2018-01-04T19:19:14Z

bors r+

155: Node tree rewrite r=vitvakatu a=kvark Fixes #44 Core of the change is switching relationships from "child->parent" to "parent->first_child->sibling", which gives us the following benefits: - clean children lifetimes, as expected by users - no need to store all the meshes returned by gltf/obj loaders: perhaps, we can follow-up with a PR that allows querying meshes by name from their parents, so that we don't even return all the meshes directly in the loader API - nice `Group::add(&something)`, like Three-js does - make `Scene` truly unique and node-less - no need for unique scene IDs ~~- technically, objects can now be present in multiple scenes at once~~ - immutable node data upon traversal (!): - we just compute the world visibility/transform on the fly - thus, no need for a separate `update_transforms` step - uses pretty iterators ;) - as a downside, not as straightforward to reflect the world info to the user (see TODO) Has other changes making things nicer: - refactored frame render code - no `&mut self` on objects - idiomatic hub indexing - safer group/camera/text/audio spawning (internal) - more robust message handling (panic on unprocessed/unexpected messages) - can debug print the `Operation` TODO: - [x] check/fix docs - [x] compute world transform/visibility for the user if needed (is it needed?) - [x] provide API to detach from parent - [x] reviews! - [x] profile the cost of recomputing the world transforms

kvark · 2018-01-04T19:21:48Z

bors r-
Almost forgot to fix the text!!!

bors · 2018-01-04T19:21:49Z

Canceled

kvark · 2018-01-04T19:26:59Z

bors r=vitvakatu

155: Node tree rewrite r=vitvakatu a=kvark Fixes #44 Core of the change is switching relationships from "child->parent" to "parent->first_child->sibling", which gives us the following benefits: - clean children lifetimes, as expected by users - no need to store all the meshes returned by gltf/obj loaders: perhaps, we can follow-up with a PR that allows querying meshes by name from their parents, so that we don't even return all the meshes directly in the loader API - nice `Group::add(&something)`, like Three-js does - make `Scene` truly unique and node-less - no need for unique scene IDs ~~- technically, objects can now be present in multiple scenes at once~~ - immutable node data upon traversal (!): - we just compute the world visibility/transform on the fly - thus, no need for a separate `update_transforms` step - uses pretty iterators ;) - as a downside, not as straightforward to reflect the world info to the user (see TODO) Has other changes making things nicer: - refactored frame render code - no `&mut self` on objects - idiomatic hub indexing - safer group/camera/text/audio spawning (internal) - more robust message handling (panic on unprocessed/unexpected messages) - can debug print the `Operation` TODO: - [x] check/fix docs - [x] compute world transform/visibility for the user if needed (is it needed?) - [x] provide API to detach from parent - [x] reviews! - [x] profile the cost of recomputing the world transforms

bors · 2018-01-04T19:52:47Z

Build succeeded

continuous-integration/travis-ci/push

kvark requested review from vitvakatu and alteous January 3, 2018 15:57

vitvakatu reviewed Jan 3, 2018

View reviewed changes

alteous reviewed Jan 3, 2018

View reviewed changes

src/scene.rs Outdated

use std::sync::MutexGuard;

/// Unique identifier for a scene.

pub type Uid = usize;

Copy link

Member

alteous Jan 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

alteous reviewed Jan 3, 2018

View reviewed changes

kvark added 4 commits January 4, 2018 10:57

Rewrite the internal node graph

454150f

Hub indexing

20b612c

Hub spawn/process refactor

ca25f9c

Relax mut self on Object

4854122

kvark force-pushed the parent branch from f180576 to 1c7659d Compare January 4, 2018 16:07

kvark added 2 commits January 4, 2018 11:24

Implement child removal

1028feb

World resolves

475c292

kvark force-pushed the parent branch from 1c7659d to 475c292 Compare January 4, 2018 16:24

alteous reviewed Jan 4, 2018

View reviewed changes

alteous approved these changes Jan 4, 2018

View reviewed changes

vitvakatu reviewed Jan 4, 2018

View reviewed changes

vitvakatu approved these changes Jan 4, 2018

View reviewed changes

Refine the internal walking API, add one layer to the group example

dce3491

kvark force-pushed the parent branch from b1d3e7c to dce3491 Compare January 4, 2018 19:18

Switch UI text rendering to be scene-independent

caf1afd

bors bot merged commit caf1afd into master Jan 4, 2018

kvark deleted the parent branch January 4, 2018 19:54

Node tree rewrite #155

Node tree rewrite #155

Conversation

kvark commented Jan 3, 2018 • edited Loading

vitvakatu left a comment

Choose a reason for hiding this comment

vitvakatu Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alteous left a comment

Choose a reason for hiding this comment

kvark commented Jan 4, 2018

kvark commented Jan 4, 2018

kvark commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alteous left a comment

Choose a reason for hiding this comment

vitvakatu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitvakatu left a comment

Choose a reason for hiding this comment

vitvakatu commented Jan 4, 2018

kvark commented Jan 4, 2018

bors bot commented Jan 4, 2018

Canceled

kvark commented Jan 4, 2018

bors bot commented Jan 4, 2018

Build succeeded

kvark commented Jan 3, 2018 •

edited

Loading

vitvakatu Jan 3, 2018 •

edited

Loading