Resolve internal object data with SyncGuard

[randomPoison]

This PR increases the utility of SyncGuard by increasing the information that it can be used to retrieve. In addition to retrieving the Node data for any object, it is now possible to:

• Downcast a Base to its concrete object type.
• Retrieve internal state data for various object types (e.g. the list of children for Group, the projection for Camera, etc.).
• Walk the hierarchy of objects under a Group.
• Search the hierarchy of objects under a Group for specific objects based on their name or type.

This functionality is especially important in conjunction with the Template functionality added in #203, as this now allows users to retrieve the objects in a template instance, which drastically increases the utility of templates to create re-usable object hierarchies.

Changes

• Adds a new trait DowncastObject, which marks object types that can be downcast from Base.
  • This is done as a new trait (instead of adding the functionality to Object) so that it could be implemented for all type but Base. This makes attempting to downcast to Base a compile error, since doing so is a meaningless operation.
  • Implements DowncastObject for all existing Object types in three-rs.
• Adds Data type member to Object, and adds new trait method resolve_data which allows an object to retrieve its internal data using a SyncGuard.
• Updates the three_object! macro to specify type Data = () and provide a basic implementation for resolve_data.
• Adds a macro derive_DowncastObject!, which provides a shorthand for implementing DowncastObject for internal types.
• Adds ObjectType enum, which represents all of the possible concrete objects types that a Base can be downcast to.
• Adds the following new methods to SyncGuard:
  • resolve_data
  • walk_hierarchy
  • find_child_by_name
  • find_children_by_name
  • find_child_of_type
  • find_children_of_type
  • find_child_of_type_by_name
  • find_children_of_type_by_name
  • downcast
• Removes Camera::projection and replace SubNode::Empty with SubNode::Camera(Projection). Moving the projection data for a camera into the Hub ensures that there is a single, canonical projection for the Camera, even if there are multiple (or zero) concrete instances of the camera in existence (i.e. the camera was a added to a group/scene and then dropped).
• Adds Camera::set_projection to allow users to update a camera's projection.
• Adds functionality to WalkedNode and TreeWalker to keep track of the NodePointer for each node, allowing us to reconstruct the Base object for walked nodes.
• Removes Directional::shadow, since it was already duplicating the internal shadow data tracked in the Hub.
• Adds new structs LightData, for retrieving the internal data for Point, Ambient, and Directional; And HemisphereLightData, for retrieving the internal data for Hemisphere.
• Marks three_object! with #[macro_export] and adds support for the default variant described in its documentation.

---

This change is 

[randomPoison]

Looks like the build failed during apt-get for reasons unrelated to my PR. Could someone with access restart it, please?

[kvark]

Great work! I got a few non-blocking concerns below:

---

Review status: all files reviewed at latest revision, all discussions resolved.

---

examples/gltf-pbr-shader.rs, line 30 at r1 (raw file):

    // rendering.
    let cam = {
        let guard = win.scene.sync_guard();

it shouldn't need a separate guard scope, right? e.g.

let cam = win.scene
  .sync_guard()
  .find_child_of_type(..);

---

src/audio.rs, line 120 at r1 (raw file):

three_object!(Source::object);

impl DowncastObject for Source {

didn't you introduce a macro to implement it?

---

src/camera.rs, line 63 at r1 (raw file):

use hub::{Hub, Operation, SubNode};
use object::{self, DowncastObject, Object, ObjectType};

would be great to avoid using self if we also use concrete types. I.e. I think it's clean to either import object, or object::self with traits, or all of the concrete types without self.

---

src/camera.rs, line 138 at r1 (raw file):

impl DowncastObject for Camera {
    fn downcast(object_type: ObjectType) -> Option<Self> {

I wonder if we should move to TryFrom when it stabilizes

---

src/light.rs, line 4 at r1 (raw file):

use gfx;
use object::{self, Object, ObjectType};

similar nit about imports here (and likely in other modules)

---

src/light.rs, line 149 at r1 (raw file):

        let node = &sync_guard.hub[self];

        match node.sub_node {

here and in all other places, let's have a shorter implementation

match sync_guard.hub[self].sub_node {
  SubNode::Light(..) => ...,
  other => panic!("`Hemisphere` had a bad sub node type: {:?}", other),
}

---

src/object.rs, line 215 at r1 (raw file):

        match &node.sub_node {
            // TODO: Handle resolving cameras better (`Empty` is only used for cameras).
            SubNode::Camera(..) => ObjectType::Camera(Camera {

hmm, this doesn't look elegant, but I don't see a better way yet

---

src/scene.rs, line 215 at r1 (raw file):

    /// and so return `()`.
    ///
    /// [`Base`]: ../object/struct.Base.html

wow, super docs!

---

src/scene.rs, line 240 at r1 (raw file):

    ///
    /// [`Group`]: ../struct.Group.html
    pub fn walk_hierarchy(&'a self, root: &Group) -> impl Iterator<Item = Base> + 'a {

nice use of impl trait!

---

src/scene.rs, line 257 at r1 (raw file):

    ///
    /// [`Base`]: ../object/struct.Base.html
    pub fn find_child_by_name(&self, root: &Group, name: &str) -> Option<Base> {

Isn't this essentially equivalent to find_children_by_name(...).next()? Not sure if it's worth having such a trivial wrapper

---

Comments from Reviewable

[randomPoison]

Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

examples/gltf-pbr-shader.rs, line 30 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

it shouldn't need a separate guard scope, right? e.g. ```rust let cam = win.scene .sync_guard() .find_child_of_type(..); ```

Done.

---

src/audio.rs, line 120 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

didn't you introduce a macro to implement it?

Ah, good catch! I'll update this to use the macro.

---

src/camera.rs, line 63 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would be great to avoid using `self` if we also use concrete types. I.e. I think it's clean to either import `object`, or `object::self` with traits, or all of the concrete types without `self`.

Sounds good to me! I didn't do that initially because I wanted to minimize changes to existing code, but I'll go through and update the imports now.

---

src/camera.rs, line 138 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I wonder if we should move to `TryFrom` when it stabilizes

Absolutely! That's what I would have preferred to do if TryFrom were already stable.

---

src/light.rs, line 4 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

similar nit about imports here (and likely in other modules)

Done.

---

src/light.rs, line 149 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

here and in all other places, let's have a shorter implementation ```rust match sync_guard.hub[self].sub_node { SubNode::Light(..) => ..., other => panic!("`Hemisphere` had a bad sub node type: {:?}", other), } ```

Done.

---

src/object.rs, line 215 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

hmm, this doesn't look elegant, but I don't see a better way yet

Er, if you're referring to the comment, it can actually be removed. The comment is leftover from a previous iteration, before I replaced SubNode::Empty with SubNode::Camera. The current version of the code here is exactly the way it should be, but I had to make some changes in order to enable downcasting cameras.

---

src/scene.rs, line 215 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

wow, super docs!

Glad you approve :)

---

src/scene.rs, line 240 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nice use of impl trait!

Thanks!

---

src/scene.rs, line 257 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Isn't this essentially equivalent to `find_children_by_name(...).next()`? Not sure if it's worth having such a trivial wrapper

Yup, that's exactly what it is! I've done this for all of the "find children" methods:

• find_children_of_type returns an iterator, find_child_of_type returns the first one found.
• find_children_by_name returns an iterator, find_child_by_name returns the first one found.
• find_children_of_type_by_name returns an iterator, find_child_of_type_by_name returns the first one found.

In my experience, it's common to only care about a single child object. For example, in my own project, I use find_child_of_type_by_name a bunch to find specific objects in the template instance based on their name in the glTF file. I could do find_children_of_type_by_name().next(), but having a function that just returns the first is both more concise and more clearly conveys intent (in my opinion).

---

Comments from Reviewable

[kvark]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

src/object.rs, line 215 at r1 (raw file):

Previously, randomPoison (David LeGare) wrote…

Er, if you're referring to the comment, it can actually be removed. The comment is leftover from a previous iteration, before I replaced `SubNode::Empty` with `SubNode::Camera`. The current version of the code here is exactly the way it *should* be, but I had to make some changes in order to enable downcasting cameras.

I'm referring to the fact that we basically mirror the SubNode enum toObjectType, which doesn't seem DRY

---

src/scene.rs, line 257 at r1 (raw file):

Previously, randomPoison (David LeGare) wrote…

Yup, that's exactly what it is! I've done this for all of the "find children" methods: * `find_children_of_type` returns an iterator, `find_child_of_type` returns the first one found. * `find_children_by_name` returns an iterator, `find_child_by_name` returns the first one found. * `find_children_of_type_by_name` returns an iterator, `find_child_of_type_by_name` returns the first one found. In my experience, it's common to only care about a single child object. For example, in my own project, I use `find_child_of_type_by_name` a bunch to find specific objects in the template instance based on their name in the glTF file. I *could* do `find_children_of_type_by_name().next()`, but having a function that just returns the first is both more concise and more clearly conveys intent (in my opinion).

Alright, I agree. Any reason they aren't internally implemented by calling the more generic counterparts?

---

Comments from Reviewable

[randomPoison]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

src/object.rs, line 215 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I'm referring to the fact that we basically mirror the `SubNode` enum to`ObjectType`, which doesn't seem DRY

Yeah, I had the same feeling, but I think in practice the two are pretty different. SubNode is an implementation detail of how three-rs structures its internals, primarily focused on representing object data in a way that facilitates rendering, whereas ObjectType is mean to be a stable part of its public API and needs to match the object types that three-rs exposes to users. The result is that the variants don't line up in a lot of cases, e.g. SubNode::LightData represents all the light types with a single variant (since the renderer ultimately treats all light types mostly the same), whereas ObjectType has to have a variant for each of the four concrete light types.

---

src/scene.rs, line 257 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Alright, I agree. Any reason they aren't internally implemented by calling the more generic counterparts?

Other than find_child_by_name (which was on oversight on my part, woops), they should be. Now that I've updated find_child_by_name, the only "find" methods that manually walk nodes in the Hub are find_children_by_name and find_children_of_type. The other four all implemented in terms of those two.

---

Comments from Reviewable

[kvark]

Alright, thanks for resolving all the concerns! Bors r+

On Jun 4, 2018, at 23:44, David LeGare notifications@github.com wrote:

Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/object.rs, line 215 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote… Yeah, I had the same feeling, but I think in practice the two are pretty different. SubNode is an implementation detail of how three-rs structures its internals, primarily focused on representing object data in a way that facilitates rendering, whereas ObjectType is mean to be a stable part of its public API and needs to match the object types that three-rs exposes to users. The result is that the variants don't line up in a lot of cases, e.g. SubNode::LightData represents all the light types with a single variant (since the renderer ultimately treats all light types mostly the same), whereas ObjectType has to have a variant for each of the four concrete light types.

src/scene.rs, line 257 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote… Other than find_child_by_name (which was on oversight on my part, woops), they should be. Now that I've updated find_child_by_name, the only "find" methods that manually walk nodes in the Hub are find_children_by_name and find_children_of_type. The other four all implemented in terms of those two.

Comments from Reviewable

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

[randomPoison]

Looks like the merge failed or something? Is there anything I need to do to resolve this?

[kvark]

let's try again bors retry

[randomPoison]

TT__TT

[kvark]

This branch cannot be rebased due to conflicts

please rebase?

[randomPoison]

Done!

[kvark]

Bors r+

On Jun 7, 2018, at 00:07, David LeGare notifications@github.com wrote:

Done!

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

[bors[bot]]

Build failed

• continuous-integration/travis-ci/push

[kvark]

ugh:

apt-get install failed

bors retry

[bors[bot]]

Build succeeded

• continuous-integration/travis-ci/push