-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Meshlet GLTF processor #13431
base: main
Are you sure you want to change the base?
Meshlet GLTF processor #13431
Conversation
/// 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>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to not have an extra method here, and instead require asset processors to implement TypePath
? (Probably not a now thing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't think about TypePath, that might be a decent alternative.
crates/bevy_gltf/src/lib.rs
Outdated
#[cfg(feature = "meshlet")] | ||
app.register_asset_processor_with_alias::<bevy_asset::processor::LoadAndSave<RawGltfLoader, meshlet_saver::MeshletMeshGltfSaver>>( | ||
meshlet_saver::MeshletMeshGltfSaver.into(), | ||
"MeshletMeshProcessor" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
..Default::default() | ||
}) | ||
} else { | ||
panic!("glTF file contained a MeshletMesh, but the meshlet cargo feature is not enabled.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"byteOffset": glb_buffer.len(), | ||
"byteLength": meshlet_mesh_bytes.len(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these fields be nested in an instance of BufferView
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
let mut meshlet_mesh_bytes = meshlet_mesh.into_bytes().expect("TODO"); | ||
|
||
let extension = json!({ | ||
"version": MESHLET_MESH_ASSET_VERSION, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
#[cfg(feature = "meshlet_processor")] | ||
{ | ||
let meshlet_mesh = MeshletMesh::from_mesh(&mesh).expect("TODO"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO - guess this needs an appropriate expect message? Or to propagate the error?
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another error TODO.
} | ||
|
||
// Pad JSON buffer if needed | ||
let mut gltf_bytes = gltf.to_vec().expect("TODO"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another error TODO.
} | ||
} else { | ||
parent.spawn(PbrBundle { | ||
// TODO: handle missing label handle errors here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be resolved now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right you are, missed that this was part of the move.
/// 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>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-authored-by: François Mockers <mockersf@gmail.com>
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. |
Objective
Solution
TODO
Testing
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.Changelog