Meshlet GLTF processor #13431

JMS55 · 2024-05-20T00:31:30Z

Objective

Allow importing GLTF scenes (gltf + separate bin, gltf embedded, or glb) and converting all meshes to MeshletMeshes

Solution

bevy_gltf has two new features - meshlet, and meshlet_processor. The former is needed for loading processed GLTF files containing MeshletMeshes. The latter is needed for the conversion process (with the asset_processor feature).
RawGltfLoader - Similiar to GltfLoader, but returns the underlying gltf-rs Gltf json structure in a RawGltf, instead of parsing it into bevy's Gltf type (which stores handles to meshes/materials/images/etc).
MeshletMeshGltfSaver - Takes a RawGltf in, converts each mesh primitive to a MeshletMesh, adds the BEVY_meshlet_mesh extension to the primitive which points to a slice within buffer 0 (the GLB BIN buffer), and then appends the serialized MeshletMesh to that slice of the buffer. Then saves the file as a GLB.
Added asset processor aliases, so that users can write "MeshletMeshProcessor" in their meta files instead of the very verbose underlying LoadAndSave type path.

TODO

Remove the original mesh data from the processed asset to save a significant amount of bytes (can only be done with single-file embedded GLTF or GLB files)
Do we need logs in the asset saver? It takes a significant amount of time, and the asset server itself does not seem to log that something is busy processing.
Handle other types of meshes better? Should we automatically skip meshes with alpha blended or masked materials which are unsupported by the meshlet feature? Should we automatically remove extra vertex attributes like vertex colors, or should it be an error (MeshletMesh only supports a a pre-defined set of vertex attributes)?
Should bevy_gltf meshlet_processor feature imply bevy_asset asset_processor? How should the features be setup? I'm not entirely sure the current setup is correct across bevy_pbr/bevy_gltf/bevy_internal/bevy.
Using single-file embedded GLTF's means that images are embedded in the processed file, instead of being separate files that can be run through the asset processor themselves.
Performance. There's definitely a lot that could be improved/parallelized, both in the asset saver and in MeshletMesh::from_mesh().

Testing

Did you test these changes? If so, how?
- Tested on bistro and the bunny
Are there any parts that need more testing?
- There's a lot of permutations that need testing:
- GLTF + separate bin containing mesh data (not recommended, but probably needs testing and maybe a warning in the asset saver)
- GLTF with embedded bin
- GLB
- All the above, with various combinations of meshes (different unsupported vertex attributes, extensions like Blender_bevy_components_workflow, etc)
- Need to test that bevy_gltf can be compiled individually with the meshlet/meshlet_processor features turned on or off. Feature flags are hard to get right, I might have missed something.
- Forgetting to add the MeshletPlugin
- Need to make sure existing examples and asset workflows did not break.
How can other people (reviewers) test your changes? Is there anything specific they need to know?
- Copy the meta file from assets/models/bunny.gltf.meta. Run bevy with features asset_processor,meshlet_processer, and make sure to both add the MeshletPlugin to your app, and set AssetMode::Processed in AssetPlugin. See the meshlet example for more details.
- If you want to spawn an entire GLTF scene instead of pulling out a single MeshletMesh, spawn a SceneBundle using foo.gltf#Scene0. Same exact API as how it works for non-meshlet mesh GLTFs.

Changelog

Added bevy::gltf RawGltf and RawGltfLoader
bevy::gltf::GltfLoaderSettings is now marked as #[serde(default)]
Added AssetApp and AssetProcessor register_asset_processor_with_alias()

…gltf

ricky26 · 2024-05-24T09:18:44Z

crates/bevy_asset/src/lib.rs

+    /// Registers the given `processor` in the [`App`]'s [`AssetProcessor`] along with an extra alias.
+    ///
+    /// This alias can be used in meta files to refer to this asset processor without using the full type name.
+    fn register_asset_processor_with_alias<P: Process>(


Would it be better to not have an extra method here, and instead require asset processors to implement TypePath? (Probably not a now thing.)

Didn't think about TypePath, that might be a decent alternative.

ricky26 · 2024-05-24T19:03:49Z

crates/bevy_gltf/src/lib.rs

+        #[cfg(feature = "meshlet")]
+        app.register_asset_processor_with_alias::<bevy_asset::processor::LoadAndSave<RawGltfLoader, meshlet_saver::MeshletMeshGltfSaver>>(
+                meshlet_saver::MeshletMeshGltfSaver.into(),
+                "MeshletMeshProcessor"


I think it would be a good idea to still include Gltf & the bevy namespace here (maybe like bevy::asset::GltfMeshletMeshProcessor?) - it's still going to be a lot shorter than the 3 type paths without the alias.

Technically bevy::asset would be inconsistent with the other auto generated processor names which use bevy_asset due to using std::any::type_name(), but I get what you're saying.

ricky26 · 2024-05-24T19:42:19Z

crates/bevy_gltf/src/loader.rs

+                                ..Default::default()
+                            })
+                        } else {
+                            panic!("glTF file contained a MeshletMesh, but the meshlet cargo feature is not enabled.")


I don't think that a panic is appropriate here since it doesn't represent a code bug. A warning & skipping this primitive, or returning an error for the entire load might be preferable.

ricky26 · 2024-05-24T20:21:25Z

crates/bevy_gltf/src/meshlet_saver.rs

+                    "byteOffset": glb_buffer.len(),
+                    "byteLength": meshlet_mesh_bytes.len(),


Should these fields be nested in an instance of BufferView?

I could, it's just more work though, and I'd need a custom type and accessory anyways. I don't see the point, it's just extra work.

Given the conversation on the other comment, I agree. I almost think it's more important to make clear that nobody should rely on this format, it's effectively an implementation detail (which is fine) - and that bevy is likely to change/remove it in later versions.

I should probably remove the version number from the error message and change the naming to magic number or something, actually. The only purpose of it is to tell users "bevy changed, you need to regenerate your assets".

ricky26 · 2024-05-24T20:25:43Z

crates/bevy_gltf/src/meshlet_saver.rs

+                let mut meshlet_mesh_bytes = meshlet_mesh.into_bytes().expect("TODO");
+
+                let extension = json!({
+                    "version": MESHLET_MESH_ASSET_VERSION,


Not a problem for now, but: is the meshlet data a well-defined/version-safe format, or is it going to be tied to engine version and isn't designed for interchange?

If it's the latter, pushing it into gltf doesn't seem like the right answer. In the long run it would be best if bevy had binary formats for processed assets which were tied to the engine version & needed very few checks/very little processing before use. Obviously that's very inconvenient for checked-in examples / tests and I don't know how to square that circle.

It's tied to the engine version. It's an opaque blob meant to be readable only by bevy's renderer with the same version that produced it in the first place.

Agreed GLTF isn't the best, I'd rather it be a bevy scene, but for now GLTF is the only workable option until scenes can reference other assets and the asset system can handle subassets better.

ricky26 · 2024-05-24T21:54:00Z

crates/bevy_gltf/src/meshlet_saver.rs

+
+                #[cfg(feature = "meshlet_processor")]
+                {
+                    let meshlet_mesh = MeshletMesh::from_mesh(&mesh).expect("TODO");


TODO - guess this needs an appropriate expect message? Or to propagate the error?

ricky26 · 2024-05-24T21:54:08Z

crates/bevy_gltf/src/meshlet_saver.rs

+        for mesh in &mut gltf.meshes {
+            for primitive in &mut mesh.primitives {
+                let meshlet_mesh = meshlet_meshes.pop_front().unwrap();
+                let mut meshlet_mesh_bytes = meshlet_mesh.into_bytes().expect("TODO");


Another error TODO.

ricky26 · 2024-05-24T21:54:17Z

crates/bevy_gltf/src/meshlet_saver.rs

+        }
+
+        // Pad JSON buffer if needed
+        let mut gltf_bytes = gltf.to_vec().expect("TODO");


Another error TODO.

ricky26 · 2024-05-24T21:55:07Z

crates/bevy_gltf/src/loader.rs

+                        }
+                    } else {
+                        parent.spawn(PbrBundle {
+                            // TODO: handle missing label handle errors here?


Does this need to be resolved now?

Don't think so. There shouldn't ever be a missing label error afaik, as they're created earlier in the function. This is also an existing comment, it just looks new because I moved the code blocks around a bit.

Right you are, missed that this was part of the move.

cart · 2024-05-28T18:26:11Z

crates/bevy_asset/src/lib.rs

+    /// Registers the given `processor` in the [`App`]'s [`AssetProcessor`] along with an extra alias.
+    ///
+    /// This alias can be used in meta files to refer to this asset processor without using the full type name.
+    fn register_asset_processor_with_alias<P: Process>(


Still on vacation so this is just a quick review: I think this approach is solid, especially in the short term. I do think migrating to TypePath + supporting short names is the right long term move (this code was written in a pre-TypePath-merge world), but even in that case, it doesn't fully solve the "generic flattening / LoadAndSave erasure problem". We'd need to use that in combination with something like what @JMS55 suggested awhile back on discord (define a new short type name that shims LoadAndSave<X, Y>). Rather than discuss whether or not that is the "right" final approach here, we should just merge "aliases" as they are simple and they get the job done.

Random musings: to support the more general "one to many asset transformations" space, we'll need to make meta serialization more type erased + dynamic than it currently is. That might also allow us to rethink how things like LoadAndSave are expressed.

…gltf

crates/bevy_gltf/src/loader.rs

Co-authored-by: François Mockers <mockersf@gmail.com>

alice-i-cecile · 2024-06-04T00:31:15Z

I've discussed this PR with Jasmine. Our conclusion is that getting this into a genuinely nice state for 0.14 isn't feasible. Furthermore, I'm worried that this isn't a good candidate for Bevy's first real pre-processing workflow and is going to be much less fraught to tackle after we've nailed down the basics.

JMS55 added 11 commits May 15, 2024 11:31

Misc asset code quality and docs

3fbc508

Merge branch 'main' of https://github.com/bevyengine/bevy

6cf0bee

WIP

b267e0c

Add TODO

f6e6554

More WIP

cfaa541

WIP

d428c3c

WIP

ab34446

WIP

8915c32

WIP

d4e6567

WIP

6ee5f47

WIP

896eae8

JMS55 added A-Assets Load files from disk to use for things like images, models, and sounds A-Scenes Serialized ECS data stored on the disk labels May 20, 2024

JMS55 added 2 commits May 19, 2024 18:00

Merge commit '2aed777435d26c357ed71cdb8c7b858de35e582e' into meshlet-…

bd258c2

…gltf

Misc merge fix

70ea4a6

JMS55 added this to the 0.15 milestone May 20, 2024

alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Uncontroversial This work is generally agreed upon labels May 21, 2024

JMS55 added 5 commits May 21, 2024 18:42

Load meshlet meshes when loading entire gltf scene

7d9b3d7

Fmt

842291a

Fix scene loading

8857188

Switch to embedded GLB buffer

b7e684f

Add processor alias

920bbdd

JMS55 modified the milestones: 0.15, 0.14 May 24, 2024

JMS55 added 4 commits May 24, 2024 10:24

Merge commit 'ec01c2dc4562adb979cf5497a48b53efa8250901' into meshlet-…

87fe640

…gltf

Rename meshlet module

dca38b8

Documentation

21105cb

Use serde default for GltfLoaderSettings

f630f1d

JMS55 added 2 commits May 24, 2024 11:11

Fix missing cfg

275e3f2

Fix compile

baf783c

JMS55 removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label May 24, 2024

JMS55 marked this pull request as ready for review May 24, 2024 18:23

ricky26 suggested changes May 24, 2024

View reviewed changes

Make some meshlet stuff private

faa0b79

alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 26, 2024

Change some docs

a7034cd

cart reviewed May 28, 2024

View reviewed changes

JMS55 added 7 commits May 28, 2024 21:49

Doc tweak

9e96e0b

Use full path for alias

0d99c8b

Fix bug with buffer slices

234a5a0

Add docs about release mode

dd17657

Merge commit '29d6575e222abbe4bdb4f5cec79377bc8a1c4cf5' into meshlet-…

34e08ef

…gltf

Add doc comment after rebase

9035d4d

Box large variant

179822a

mockersf reviewed May 29, 2024

View reviewed changes

crates/bevy_gltf/src/loader.rs Outdated Show resolved Hide resolved

JMS55 and others added 2 commits May 29, 2024 10:10

Update crates/bevy_gltf/src/loader.rs

b360a3b

Co-authored-by: François Mockers <mockersf@gmail.com>

Sharper shadows

cf7595d

JMS55 mentioned this pull request May 29, 2024

Meshlet tracking issue #11518

Open

40 tasks

JMS55 added 2 commits May 29, 2024 13:21

Use glb bunny asset

4557879

Dynamic shadow config in example

c746a5f

alice-i-cecile removed this from the 0.14 milestone Jun 4, 2024

alice-i-cecile removed the S-Needs-SME Decision or review from an SME is required label Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Meshlet GLTF processor #13431

Meshlet GLTF processor #13431

JMS55 commented May 20, 2024 •

edited

ricky26 May 24, 2024

JMS55 May 25, 2024

ricky26 May 24, 2024

JMS55 May 25, 2024 •

edited

ricky26 May 24, 2024

ricky26 May 24, 2024

JMS55 May 25, 2024

ricky26 May 25, 2024

JMS55 May 25, 2024

ricky26 May 24, 2024

JMS55 May 25, 2024

ricky26 May 24, 2024

ricky26 May 24, 2024

ricky26 May 24, 2024

ricky26 May 24, 2024

JMS55 May 25, 2024 •

edited

ricky26 May 25, 2024

cart May 28, 2024 •

edited

alice-i-cecile commented Jun 4, 2024

		"byteOffset": glb_buffer.len(),
		"byteLength": meshlet_mesh_bytes.len(),

Meshlet GLTF processor #13431

Are you sure you want to change the base?

Meshlet GLTF processor #13431

Conversation

JMS55 commented May 20, 2024 • edited

Objective

Solution

TODO

Testing

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JMS55 May 25, 2024 • edited

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

JMS55 May 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cart May 28, 2024 • edited

Choose a reason for hiding this comment

alice-i-cecile commented Jun 4, 2024

JMS55 commented May 20, 2024 •

edited

JMS55 May 25, 2024 •

edited

JMS55 May 25, 2024 •

edited

cart May 28, 2024 •

edited