feat: Load full scenes from GLTF files #435

Rhuagh · Oct 20, 2017

Only have support for static scenes so far. Need to complete the Animation PR before I can finish loading that.

Also waiting on multiple features in other parts of the engine before I can add full material support, indexed meshes etc.

torkleyy · Oct 20, 2017

torkleyy requested changes Oct 20, 2017

View changes

This looks fantastic, thank you so much!

torkleyy · Oct 20, 2017

Cargo.toml

 [workspace]
-members = ["amethyst_assets", "amethyst_audio", "amethyst_config", "amethyst_core", "amethyst_input", "amethyst_renderer", "amethyst_utils"]
+members = ["amethyst_assets", "amethyst_audio", "amethyst_config", "amethyst_core", "amethyst_gltf", "amethyst_input",


We should eventually make this a virtual workspace. I think those support kind of implicit members, not sure.

No idea tbh :)

torkleyy · Oct 20, 2017

amethyst_gltf/Cargo.toml

+travis-ci = { repository = "amethyst/amethyst" }
+
+[dependencies]
+amethyst_assets = { path = "../amethyst_assets/" }


You need to specify the versions here.

torkleyy · Oct 20, 2017

amethyst_gltf/Cargo.toml

+amethyst_assets = { path = "../amethyst_assets/" }
+amethyst_renderer = { path = "../amethyst_renderer/" }
+amethyst_core = { path = "../amethyst_core/" }
+base64 = "*"


Please don't use * in library crates

(This actually breaks the cargo upload too)

Ah. Missed that one, sorry

torkleyy · Oct 20, 2017

amethyst_gltf/src/format/importer.rs

+            let format = match mime_type {
+                "image/png" => ImageFormat::Png,
+                "image/jpeg" => ImageFormat::Jpeg,
+                _ => unreachable!(),


Technically, this could also be e.g. .bmp

No support in gltf got other than jpg and bmp

Jpg and png I mean ofc

Is that defined by reference or the Rust implementation?

The glTF specification allows only JPEG and PNG.

torkleyy · Oct 20, 2017

amethyst_gltf/src/format/importer.rs

+                let format = match ty {
+                    "image/png" => ImageFormat::Png,
+                    "image/jpeg" => ImageFormat::Jpeg,
+                    _ => unreachable!(),


torkleyy · Oct 20, 2017

amethyst_gltf/src/format/mod.rs

+/// Gltf scene format, will cause the whole default scene to be loaded from the given file.
+///
+/// Using the `GltfSceneLoaderSystem` a `Handle<GltfSceneAsset>` from this format can be attached
+/// to an entity in ECS, and the system will then load the full scene using the given entity


Good idea 👍

This could probably be replaced by a real prefab system at some point

More like generalized into prefab system

torkleyy · Oct 20, 2017

amethyst_gltf/src/format/mod.rs

+        options: GltfSceneOptions,
+    ) -> Result<GltfSceneAsset, BoxedErr> {
+        let gltf = load_gltf(source, &name, options).map_err(|err| BoxedErr::new(err))?;
+        match gltf.default_scene {


Should be

if gltf.default_scene.is_some() || gltf.scenes.len() == 1 { Ok(gltf) } else { Err(..) }

torkleyy · Oct 20, 2017

amethyst_gltf/src/systems.rs

+
+/// A GLTF scene loader, will transform `Handle<GltfSceneAsset>` into full entity hierarchies.
+///
+/// Will also do the asset storage processing for `GltfSceneAsset`.s


"GltfSceneAsset.s"

torkleyy · Oct 20, 2017

amethyst_gltf/src/systems.rs

+
+impl GltfSceneLoaderSystem {
+    pub fn new() -> Self {
+        Self {}


Meh, I don't like this syntax. And I guess we don't need this constructor, do we?

Remnant from when it had fields

torkleyy · Oct 20, 2017

amethyst_gltf/src/systems.rs

+    // TODO: emissive factor
+    // TODO: alpha
+    // TODO: double sided
+    let albedo = match material.base_color.0.handle {


I think you could use .cloned().unwrap_or(..)

Possible, I'll check

@alteous This is our gltf integration PR in case you're interested.

torkleyy · Oct 20, 2017

@alteous This is our gltf integration PR in case you're interested.

alteous · Oct 20, 2017

alteous reviewed Oct 20, 2017

View changes

alteous · Oct 20, 2017

amethyst_gltf/src/systems.rs

+}
+
+// Load a single node, attach all data to the given `node_entity`.
+fn load_node(


alteous · Oct 20, 2017

alteous reviewed Oct 20, 2017

View changes

alteous · Oct 20, 2017

amethyst_gltf/src/format/importer.rs

@@ -0,0 +1,328 @@
+use std;


I hope the burden of writing your own loader wasn't too annoying. It's a compromise between being high-level and not doing 'too much' on behalf of the user.

No, it was fine, I started with using the importer to get the first implementation in place, then I basically copied the importer code and replaced the file loading parts only.

alteous · Oct 20, 2017

alteous reviewed Oct 20, 2017

View changes

alteous · Oct 20, 2017

amethyst_gltf/src/format/mod.rs

+}
+
+fn map_mode(mode: gltf::json::mesh::Mode) -> Result<Primitive, GltfError> {
+    use gltf::json::mesh::Mode::*;


I suggest gltf::mesh::Mode instead. The gltf::json module is deprecated and will be removed in 1.0.

Oh, I must have missed that, it did feel weird to have to go down to the json stuff.

alteous · Oct 20, 2017

alteous reviewed Oct 20, 2017

View changes

alteous · Oct 20, 2017

amethyst_gltf/src/format/mod.rs

+}
+
+impl Format<GltfSceneAsset> for GltfSceneFormat {
+    const NAME: &'static str = "GLTF";


Also, .glb for binary glTF - not sure if you still distinguish by extension.

This is just the name of the format, not related to extension at all.

torkleyy · Oct 21, 2017

torkleyy requested changes Oct 21, 2017

View changes

Thanks for addressing, got a few other concerns but otherwise it's great!

torkleyy · Oct 21, 2017

amethyst_gltf/src/format/importer.rs

+    }
+}
+
+fn import_standard<'a>(


Why an explicit lifetime here?

Ah, used to take a reference to something that needed it

torkleyy · Oct 21, 2017

amethyst_gltf/src/format/importer.rs

+    let gltf = validate_standard(unvalidated)?;
+    let bin = None;
+    let mut buffers = Buffers(vec![]);
+    for buffer in load_external_buffers(source, base_path, &gltf, bin)? {


Why are you iterating over a vec just to push the elements to another, intially empty one?

torkleyy · Oct 21, 2017

amethyst_gltf/src/lib.rs

+    pub generate_tex_coords: Option<(f32, f32)>,
+}
+
+impl Default for GltfSceneOptions {


Can't we derive this?

torkleyy · Oct 21, 2017

amethyst_gltf/src/systems.rs

+/// A GLTF scene loader, will transform `Handle<GltfSceneAsset>` into full entity hierarchies.
+///
+/// Will also do the asset storage processing for `GltfSceneAsset`.
+pub struct GltfSceneLoaderSystem;


Please add a dummy field in case we want to add fields later

What is the recommended type for a dummy field?

() I guess

torkleyy · Oct 21, 2017

amethyst_gltf/src/systems.rs

+                // process new texture handles, cache them in the textures
+                for (material_index, texture_index, handle) in texture_handles {
+                    match texture_index {
+                        0 => {


Could we at least use constants somewhere, this is a bit magic

torkleyy · Oct 21, 2017

amethyst_gltf/src/systems.rs

+                // Use the default scene if set, otherwise use the first scene.
+                // Note that the format will throw an error if the default scene is not set,
+                // and there are more than one scene in the GLTF, so defaulting to scene 0 is safe
+                let scene_index = match scene_asset.default_scene {


Should use unwrap_or

torkleyy · Oct 21, 2017

amethyst_gltf/src/systems.rs

+    };
+
+    // Load material for the primitive
+    let material = match primitive.material {


I think it would be nicer to do this:

let material = primitive .material .map(|i| (i, scene_asset.materials.get(i))) .and_then(|(i, m)| load_material(i, m, ..)) .unwrap_or_else(|| mat_defaults.0.clone());

Hmm, i need to do unwrap on m in and_then then

If I reverse it it works though.

torkleyy · Oct 21, 2017

torkleyy approved these changes Oct 21, 2017

View changes

Looks great!

omni-viral · Oct 21, 2017

omni-viral approved these changes Oct 21, 2017

View changes

Great job @Rhuagh !

Xaeroxe · Oct 21, 2017

Xaeroxe approved these changes Oct 21, 2017

View changes

I've got a couple organization nits but otherwise this looks awesome! Thanks for your work!

...I'm mostly just glad it wasn't me that had to write this :P Looks like loads of data to manage.

Xaeroxe · Oct 21, 2017

amethyst_gltf/src/format/importer.rs

+where
+    P: AsRef<Path>,
+{
+    import_impl(source, path.as_ref())


You could replace this with the body of import_impl and just add

let path = path.as_ref()

at the top.

Xaeroxe · Oct 21, 2017

amethyst_gltf/src/format/importer.rs

+}
+
+fn read_to_end<P: AsRef<Path>>(source: Arc<AssetSource>, path: P) -> Result<Vec<u8>, BoxedErr> {
+    read_to_end_impl(source, path.as_ref())


Same as above.

bors r+

Xaeroxe · Oct 21, 2017

bors r+

Build succeeded

bors · Oct 21, 2017

Build succeeded

Rhuagh added project: assets project: rendering type: feature status: ready labels Oct 20, 2017

Rhuagh requested review from omni-viral, Xaeroxe and torkleyy Oct 20, 2017

torkleyy self-requested a review Oct 21, 2017

Rhuagh requested a review from torkleyy Oct 21, 2017

feat: Load full scenes from GLTF files

Loading status checks…

0b738f9

bors bot merged commit 0b738f9 into amethyst:develop Oct 21, 2017
3 checks passed

3 checks passed

bors Build succeeded

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Rhuagh deleted the Rhuagh:gltf-format branch Oct 21, 2017

amethyst/amethyst

Join GitHub today

feat: Load full scenes from GLTF files #435

Conversation

Rhuagh commented Oct 20, 2017 • edited Edited 1 time Rhuagh edited Oct 20, 2017 (most recent)

Rhuagh added project: assets project: rendering type: feature status: ready labels Oct 20, 2017

Rhuagh requested review from omni-viral, Xaeroxe and torkleyy Oct 20, 2017

torkleyy requested changes Oct 20, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy commented Oct 20, 2017

alteous reviewed Oct 20, 2017 View changes

This comment has been minimized.

alteous reviewed Oct 20, 2017 View changes

This comment has been minimized.

alteous Oct 20, 2017 • edited Edited 1 time alteous edited Oct 20, 2017 (most recent)

This comment has been minimized.

alteous reviewed Oct 20, 2017 View changes

This comment has been minimized.

This comment has been minimized.

alteous reviewed Oct 20, 2017 View changes

This comment has been minimized.

alteous Oct 20, 2017 • edited Edited 1 time alteous edited Oct 20, 2017 (most recent)

This comment has been minimized.

torkleyy self-requested a review Oct 21, 2017

torkleyy requested changes Oct 21, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Rhuagh Oct 21, 2017 • edited Edited 1 time Rhuagh edited Oct 21, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Rhuagh requested a review from torkleyy Oct 21, 2017

torkleyy approved these changes Oct 21, 2017 View changes

omni-viral approved these changes Oct 21, 2017 View changes

Xaeroxe approved these changes Oct 21, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Rhuagh commented Oct 20, 2017 •

edited

Edited 1 time

Rhuagh edited Oct 20, 2017 (most recent)

torkleyy requested changes Oct 20, 2017

View changes

alteous reviewed Oct 20, 2017

View changes

alteous reviewed Oct 20, 2017

View changes

alteous Oct 20, 2017 •

edited

Edited 1 time

alteous edited Oct 20, 2017 (most recent)

alteous reviewed Oct 20, 2017

View changes

alteous reviewed Oct 20, 2017

View changes

alteous Oct 20, 2017 •

edited

Edited 1 time

alteous edited Oct 20, 2017 (most recent)

torkleyy requested changes Oct 21, 2017

View changes

Rhuagh Oct 21, 2017 •

edited

Edited 1 time

Rhuagh edited Oct 21, 2017 (most recent)

torkleyy approved these changes Oct 21, 2017

View changes

omni-viral approved these changes Oct 21, 2017

View changes

Xaeroxe approved these changes Oct 21, 2017

View changes