Template System and Reworked glTF Handling

[randomPoison]

This PR adds a new system for creating templates for hierarchies of objects. Templates describe many objects and their relationships to each other, and allow the user to instantiate the entire hierarchy of objects at once. Templates can be instantiated an arbitrary number of times. All instances of a template will use the same GPU resources for each instance of a mesh, reducing duplication of GPU resources and allowing for efficient instanced rendering.

• Adds a new module template with type declarations for Template and its various sub-objects.
• Adds Factory::instantiate_template for creating concrete objects from a template.
• Adds Factory::instanced_geometry to allow users to upload a Geometry to the GPU (this is needed to allow templates to reuse GPU resources).
• Removes Factory::instantiate_mesh and replace it with Factory::instanced_mesh, which allows the user to create a mesh given an InstancedGeometry and a Material. The removal of instantiate_mesh was suggested by @kvark, I opted to also add instanced_mesh as a convenience function for cases where constructing a full-blown Template is overkill.
• Reworks Factory::load_gltf to return a list of Template objects, returning one Template for each scene in the glTF file.
• Adds Factory::camera as a helper method to create a Camera from a Projection. Adding this simplified some of the logic in instantiating templates, since the template stores camera projections as Projection.
• Adds the option to set an up direction for the Orbit controller. I wound up not needing this for any of the examples, but I opted to leave it in the PR because it's useful functionality to have.

Sorry this took so long. _{Hopefully this attempt sticks. I'm gonna go to bed now.}

---

This change is 

[kvark]

Amazing work! I'm happy to see this moving forward 👍
Left a few concerns/questions for discussion here.

---

Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

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

    // If we didn't find a camera in the glTF scene, create a default one to use.
    let cam = cam.unwrap_or_else(|| {

does that look ok when running the example with no parameters?

---

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

//!
//! It is often the case that you will want to have multiple instances of the same model or
//! hierarchy of object in your scene. While you could manually construct each instance yourself,

nit: hierarchy of objects
nit: rewrite to refer to "user" as opposed to "you"

---

src/template.rs, line 5 at r1 (raw file):

//! It is often the case that you will want to have multiple instances of the same model or
//! hierarchy of object in your scene. While you could manually construct each instance yourself,
//! three provides a templating system to allow you to describe your model's hierarchy ahead

make "three" distinguishable here, by either appending the -rs prefix, or applying some formatting

---

src/template.rs, line 22 at r1 (raw file):

//! information on the different node sub-types.
//!
//! Data for specific types of nodes are held in the other member arrays of [`Template`]. For

big thanks for writing the docs up front!

---

src/template.rs, line 47 at r1 (raw file):

//!
//! When setting up a mesh in a template, you must first upload your [`Geometry`] to the GPU
//! using [`Factory::instanced_geometry`]. This will give you an [`InstancedGeometry`] object

should be called instantiate_geometry since it's an expensive operation and not a simple accessor

---

src/template.rs, line 114 at r1 (raw file):

    /// All objects defined by this template.
    pub nodes: Vec<TemplateNode>,

perhaps, we could me more consistent in naming here and call it NodeTemplate?

---

src/template.rs, line 158 at r1 (raw file):

    ///
    /// This translation is relative to its parent node when instantiated.
    pub translation: mint::Point3<f32>,

perhaps, we should re-use node::Transform here

---

src/template.rs, line 210 at r1 (raw file):

/// [module documentation]: ./index.html#node-templates
#[derive(Debug, Clone)]
pub enum TemplateNodeData {

Note that we can technically simplify the structure complexity here if we reverse the relationships: have all the specific templates to just have node: usize field pointing to a node template, which becomes just a transform.
Doing this would make some things simpler, but we need to ensure we'll still be able to instantiate the hierarchy into our internal node tree

---

src/factory/mod.rs, line 574 at r1 (raw file):

    ///
    /// [`template`]: ./template/index.html#mesh-instancing
    pub fn instanced_geometry(

we need to hide this step for anything but advanced use of the engine

---

src/factory/mod.rs, line 633 at r1 (raw file):

    /// [`Mesh`]: ./struct.Mesh.html
    /// [`template`]: ./template/index.html#mesh-instancing
    pub fn instanced_mesh<M: Into<Material>>(

similarly, this should be called with a verb ("instantiate_")

---

Comments from Reviewable

[randomPoison]

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

---

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

Previously, kvark (Dzmitry Malyshau) wrote…

does that look ok when running the example with no parameters?

I've actually only tested this running it with no parameters. The Lantern file (which gets loaded by default) happens to be setup such that lantern is in direct focus of the camera's default view, so it looks good when run without parameters.

---

src/template.rs, line 47 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

should be called instantiate_geometry since it's an expensive operation and not a simple accessor

For instanced_geometry and instanced_mesh I followed the naming convention for creating other objects, e.g. Factory::group, Factory::mesh, and such. I can change them to use instantiate_ instead, but I'm not sure that makes the most sense. Is there a reason why we would use a different naming convention for these methods?

---

src/template.rs, line 210 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Note that we can technically simplify the structure complexity here if we reverse the relationships: have all the specific templates to just have node: usize field pointing to a node template, which becomes just a transform.
Doing this would make some things simpler, but we need to ensure we'll still be able to instantiate the hierarchy into our internal node tree

I'm not sure I'm following what you're suggesting. Could you clarify further, maybe provide a rough sketch of what you're imagining?

---

src/factory/mod.rs, line 574 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we need to hide this step for anything but advanced use of the engine

Agreed that this only necessary for advanced usage. Is there anything I should do to "hide" it for less advanced users?

---

Comments from Reviewable

[kvark]

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

---

src/template.rs, line 47 at r1 (raw file):

Previously, randomPoison (David LeGare) wrote…

For instanced_geometry and instanced_mesh I followed the naming convention for creating other objects, e.g. Factory::group, Factory::mesh, and such. I can change them to use instantiate_ instead, but I'm not sure that makes the most sense. Is there a reason why we would use a different naming convention for these methods?

we should probably rename everything to align better with rust conventions, e.g. create_mesh, etc

---

src/template.rs, line 210 at r1 (raw file):

Previously, randomPoison (David LeGare) wrote…

I'm not sure I'm following what you're suggesting. Could you clarify further, maybe provide a rough sketch of what you're imagining?

What I'm saying is that we can get away without the TemplateNodeData enum. For example, instead of having Mesh(usize) variant here, there could be a MeshTemplate::node field with the node index. Doing that for other XXXTemplate structs would eventually lead to removal of all variants of this enum, which means we can get away without it entirely.
Is this a little more clear?

---

src/factory/mod.rs, line 574 at r1 (raw file):

Previously, randomPoison (David LeGare) wrote…

Agreed that this only necessary for advanced usage. Is there anything I should do to "hide" it for less advanced users?

not really

---

Comments from Reviewable

[randomPoison]

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

---

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

Previously, kvark (Dzmitry Malyshau) wrote…

nit: hierarchy of objects
nit: rewrite to refer to "user" as opposed to "you"

Is there an established convention for not using second person in Rust docs? I did a quick look through the standard library docs, and it seems like second person is used pretty commonly, especially in module-level docs (e.g. std crate docs and collections module docs).

---

src/template.rs, line 5 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

make "three" distinguishable here, by either appending the -rs prefix, or applying some formatting

Done.

---

src/template.rs, line 22 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

big thanks for writing the docs up front!

No problem! _{It was only the most time consuming portion of the entire PR...}

---

src/template.rs, line 47 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we should probably rename everything to align better with rust conventions, e.g. create_mesh, etc

Ah, that's reasonable. I updated them, but I don't think it would make sense to change the other methods in this PR.

For the two methods in this PR, I went with upload_geometry (to match the language I use in the documentation) and create_instanced_mesh.

---

src/template.rs, line 114 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

perhaps, we could me more consistent in naming here and call it NodeTemplate?

Done.

---

src/template.rs, line 158 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

perhaps, we should re-use node::Transform here

Done.

---

src/template.rs, line 210 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

What I'm saying is that we can get away without the TemplateNodeData enum. For example, instead of having Mesh(usize) variant here, there could be a MeshTemplate::node field with the node index. Doing that for other XXXTemplate structs would eventually lead to removal of all variants of this enum, which means we can get away without it entirely.
Is this a little more clear?

So, if I'm understanding correctly, you're suggesting something like this:

pub struct Template {
    pub nodes: Template<Transform>,
    pub groups: Template<GroupTemplate>,

    // Members for each type of object.
}

pub struct MeshTemplate {
    pub node: usize,

    // The same members as before...
}

pub struct GroupTemplate {
    pub node: usize,

    pub children: Vec<???>,
}

That makes sense to me. My only question at this point is how do group templates specify their children?

pub children: Vec<???>,

They can't just specify children as a Vec<usize> because then there's no way to tell what type of object each child refers to. There are two options that I can think of:

• Create an enum ChildType. This would have effectively the same variants thatTemplateNodeData has, but they would at least be simplified to all wrapping a usize.
• Have a separate list of children for each type of object (e.g. mesh_children, group_children, etc.). This option feels bad to me; It makes it harder to iterate over all children of a group, and it involves multiple allocations where one would do.

Do you have a preference as to what approach to take? I'd lean towards the enum, personally. Or is there another approach that I'm not seeing?

---

src/factory/mod.rs, line 633 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

similarly, this should be called with a verb ("instantiate_")

Done.

---

Comments from Reviewable

[kvark]

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

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

Previously, randomPoison (David LeGare) wrote…

Is there an established convention for not using second person in Rust docs? I did a quick look through the standard library docs, and it seems like second person is used pretty commonly, especially in module-level docs (e.g. std crate docs and collections module docs).

Interesting findings! I'll withdraw my nit then, for now at least :)

---

src/template.rs, line 47 at r1 (raw file):

Previously, randomPoison (David LeGare) wrote…

Ah, that's reasonable. I updated them, but I don't think it would make sense to change the other methods in this PR.

For the two methods in this PR, I went with upload_geometry (to match the language I use in the documentation) and create_instanced_mesh.

sounds good

---

src/template.rs, line 210 at r1 (raw file):

Previously, randomPoison (David LeGare) wrote…

So, if I'm understanding correctly, you're suggesting something like this:

pub struct Template {
    pub nodes: Template<Transform>,
    pub groups: Template<GroupTemplate>,

    // Members for each type of object.
}

pub struct MeshTemplate {
    pub node: usize,

    // The same members as before...
}

pub struct GroupTemplate {
    pub node: usize,

    pub children: Vec<???>,
}

That makes sense to me. My only question at this point is how do group templates specify their children?

pub children: Vec<???>,

They can't just specify children as a Vec<usize> because then there's no way to tell what type of object each child refers to. There are two options that I can think of:

• Create an enum ChildType. This would have effectively the same variants thatTemplateNodeData has, but they would at least be simplified to all wrapping a usize.
• Have a separate list of children for each type of object (e.g. mesh_children, group_children, etc.). This option feels bad to me; It makes it harder to iterate over all children of a group, and it involves multiple allocations where one would do.

Do you have a preference as to what approach to take? I'd lean towards the enum, personally. Or is there another approach that I'm not seeing?

Yeah, in this approach the group would not specify it's children (so, neither of the points in your list). This would be implicit, based on which objects point to the node associated with the group. I'm not 100% sure this is feasible, but it does feel important to explore here.

---

src/factory/mod.rs, line 2 at r2 (raw file):

#[cfg(feature = "gltf-loader")]
pub(crate) mod load_gltf;

why does it need to be pub?

---

Comments from Reviewable

[randomPoison]

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

---

src/template.rs, line 210 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Yeah, in this approach the group would not specify it's children (so, neither of the points in your list). This would be implicit, based on which objects point to the node associated with the group. I'm not 100% sure this is feasible, but it does feel important to explore here.

Totally, I'm down to play in the (design) space. Let me give a concrete example to see if I'm understanding.

Let's say I want to create a template for a Group at position (1, 2, 3) and give it two children, a mesh and a light. To describe this I would create a GroupTemplate and have it's node member point to a NodeTemplate with a position at (1, 2, 3). Then, I would make a MeshTemplate and LightTemplate, and have each of their node members point to the same NodeTemplate that the GroupTemplate points to, which would implicitly mark them as being children of the group. Is that right? That's the only setup I can think of that would implicitly associate objects with a group based on their node.

Assuming that's right, how would I specify a different transform for the mesh or the node (e.g. I want the mesh to have a local position of (4, 5, 6))? As I understand it, GroupTemplate is the only one that would be able to specify it's own transform via node, for other types node would effectively specify their parent, and they would end up with a default local transform.

An alternative that occurs to me now: Instead of having groups list their children, have all the types have a group: Option<usize> which specifies the index of the group they should be added to. That would achieve your goal of being able to remove TemplateNodeData, while keeping parent/child relationships explicit (which I kind of prefer). I generally prefer to have parents list their children, rather than the other way around, but this approach would help to simplify the overall structure of the template, and has the nice benefit of reducing the number of allocations needed. It might also simplify the template instantiation code, now that I think about it 🤔

---

src/factory/mod.rs, line 2 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why does it need to be pub?

Woops, I'm pretty sure this is an artifact of an earlier iteration where I was defining types in that module that I wanted to be accessible within the crate. I'll revert this since it's not necessary.

---

Comments from Reviewable

[kvark]

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/template.rs, line 210 at r1 (raw file):

Previously, randomPoison (David LeGare) wrote…

Totally, I'm down to play in the (design) space. Let me give a concrete example to see if I'm understanding.

Let's say I want to create a template for a Group at position (1, 2, 3) and give it two children, a mesh and a light. To describe this I would create a GroupTemplate and have it's node member point to a NodeTemplate with a position at (1, 2, 3). Then, I would make a MeshTemplate and LightTemplate, and have each of their node members point to the same NodeTemplate that the GroupTemplate points to, which would implicitly mark them as being children of the group. Is that right? That's the only setup I can think of that would implicitly associate objects with a group based on their node.

Assuming that's right, how would I specify a different transform for the mesh or the node (e.g. I want the mesh to have a local position of (4, 5, 6))? As I understand it, GroupTemplate is the only one that would be able to specify it's own transform via node, for other types node would effectively specify their parent, and they would end up with a default local transform.

An alternative that occurs to me now: Instead of having groups list their children, have all the types have a group: Option<usize> which specifies the index of the group they should be added to. That would achieve your goal of being able to remove TemplateNodeData, while keeping parent/child relationships explicit (which I kind of prefer). I generally prefer to have parents list their children, rather than the other way around, but this approach would help to simplify the overall structure of the template, and has the nice benefit of reducing the number of allocations needed. It might also simplify the template instantiation code, now that I think about it 🤔

Sounds like you are on the right track :) let's follow up on Gitter with further concerns and findings

---

Comments from Reviewable

[randomPoison]

@kvark I've updated the PR to remove TemplateNodeData and instead add a parent field to NodeTemplate (which is now called ObjectTemplate). I still need to update Factory::load_gltf to account for this, but the public API has all been updated.

[kvark]

Thank you for all the hard work!
Let's get this merged. Do you think there are a few commits (where you go back and forth, for example) to squash?

[randomPoison]

Sure! I'll squash out all the unimportant commits.

[kvark]

🚀 🐑 it
bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push