-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 continuous LOD #12755
Meshlet continuous LOD #12755
Conversation
Optimise extract_meshlet_meshes and prepare_meshlet_per_frame_resources
Co-authored-by: vero <[email protected]>
Co-authored-by: vero <[email protected]>
Co-authored-by: vero <[email protected]>
This reverts commit a5626b0.
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 did a look over this, mostly for code style and so forth, not so much for the algorithm. In general this looks pretty straightforward. I definitely appreciate the clarity from the comments and splitting this up into a smaller patch!
Exciting stuff :)
fn lod_error_is_imperceptible(cp: vec3<f32>, r: f32) -> bool { | ||
let d2 = dot(cp, cp); | ||
let r2 = r * r; | ||
let sphere_diameter_uv = view.projection[0][0] * r / sqrt(d2 - r2); |
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.
let sphere_diameter_uv = view.projection[0][0] * r / sqrt(d2 - r2); | |
let sphere_diameter_uv = view.projection[0][0] * r * inverseSqrt(d2 - r2); |
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.
The spec says inverseSqrt() does not work if e <= 0. Need to think if this is an issue.
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 dont think sqrt is guaranteed to work for e <= 0 either. I'd just slab an abs or max 0 on it if its a worry, being explicit about edge case behavior intent is valuable and saturating/clamping is often free in gpus
unsafe { | ||
meshopt_optimizeMeshlet( | ||
&mut meshlets.vertices[meshlet.vertex_offset as usize], | ||
&mut meshlets.triangles[meshlet.triangle_offset as usize], | ||
meshlet.triangle_count as usize, | ||
meshlet.vertex_count as usize, | ||
); | ||
} |
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.
Can we add a safe wrapper for this to the meshopt
crate instead? It looks like the safe wrapper would be trivial, and that would let us avoid messing with the unsafe code settings in this crate.
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.
Yeah it's not published yet though, so for now, unsafe it is.
/// The total amount of triangles summed across all meshlets in the mesh. | ||
pub total_meshlet_triangles: u64, | ||
/// The total amount of triangles summed across all LOD 0 meshlets in the mesh. | ||
pub worst_case_meshlet_triangles: u64, |
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.
Maybe max_meshlet_triangles
?
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 personally feel that that's less clear. Up to maintainers.
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 prefer keeping it as is
let t = (lod_level - 1) as f32 / 19.0; | ||
let target_error = 0.1 * t + 0.01 * (1.0 - t); |
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 personally like factoring tunable parameters like this out into const
s at the top of the file so people can play with them if they need to. Up to you.
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 used to have it in a previous commit. I can't decide why I removed it. Probably because it's just a heuristic that mostly works for the meshes I tested. Might need to be tunable by users in the future, idk. It'll need further testing when we try it out on a large array of meshes, but this PR was just to get something usable.
); | ||
|
||
// Check if we were able to simplify to at least 65% triangle count | ||
if simplified_group_indices.len() as f32 / group_indices.len() as f32 > 0.65 { |
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.
Same here.
xadj.push(adjncy.len() as i32); | ||
|
||
let mut group_per_meshlet = vec![0; simplification_queue.len()]; | ||
let partition_count = (simplification_queue.len().div_ceil(4)) as i32; |
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.
Why 4?
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.
4 meshlets per group, ideally. 4 meshlets (256 triangles), simplify group to 50% (128 triangles), split back into meshlets = 2 meshlets (64 x 2 triangles).
I should probably rename this to target_partition_count, there are no hard limits.
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.
Earlier code mentions that the meshlets are grouped into roughly 4 in a group. How come 4 is always used here if there aren't guaranteed to be 4?
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 the partition count. Given N nodes (meshlets), Metis divides them into P (partition_count) partitions (groups).
So given 1000 meshlets, we tell Metis to divide the 1000 meshlets into (1000/25)=250 groups. It's not guaranteed that we have exactly 4 per group though. Some groups might have slightly more or less than 4 meshlets, as it tries to spread the cost out as defined by the edge weights between nodes. We just have to hope for an even split between all partitions, which would give 4 meshlets per group if that were to happen.
Co-authored-by: Patrick Walton <[email protected]>
Co-authored-by: Patrick Walton <[email protected]>
assets/models/bunny.meshlet_mesh
Outdated
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.
Do you think you will need to change this more? Storing binary files in git and having history of them isn't the nicest thing so if we could avoid it, that would be good. :)
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.
Yep, it will definitely change a couple more times... Storing it externally also sucks though as then you can't run the example easily... Idk what we should do.
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 will almost certainly change again, the format is not bit packed yet.
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.
If I get Gltf->MeshletMesh asset preprocessing in, we can ship the repo with the GLTF file, and just have users convert it as part of the example. That way we won't have to worry about the MeshletMesh asset changing. But that's a good bit off, I don't want to block on that.
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.
@bevyengine/maintainer-team any thoughts on this?
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.
Just to note as it happened on Discord - it was decided that hosting the file out of tree would be better, and prompting the user to download it. The reason is that binary files that are modified and committed to the repository will have all versions in git history and the only way to remove them is to rewrite the git history.”, which isn’t nice. But also, rewriting git history breaks all open PRs and branches.
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.
Just a few nits/clarifications, otherwise this looks good to me.
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 approve, but I still want to hear other maintainers' thoughts about the meshlet mesh file. Every change will go into the history and everyone not doing a shallow clone, which will be basically everyone, will download the whole history of all files. That isn't good.
# Objective - #12755 introduced the need to download a file to run an example - This means the example fails to run in CI without downloading that file ## Solution - Add a new metadata to examples "setup" that provides setup instructions - Replace the URL in the meshlet example to one that can actually be downloaded - example-showcase execute the setup before running an example
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1303 if you'd like to help out. |
Adds a basic level of detail system to meshlets. An extremely brief summary is as follows:
from_mesh.rs
, once we've built the first level of clusters, we group clusters, simplify the new mega-clusters, and then split the simplified groups back into regular sized clusters. Repeat several times (ideally until you can't anymore). This forms a directed acyclic graph (DAG), where the children are the meshlets from the previous level, and the parents are the more simplified versions of their children. The leaf nodes are meshlets formed from the original mesh.cull_meshlets.wgsl
, each cluster selects whether to render or not based on the LOD bounding sphere (different than the culling bounding sphere) of the current meshlet, the LOD bounding sphere of its parent (the meshlet group from simplification), and the simplification error relative to its children of both the current meshlet and its parent meshlet. This kind of breaks two pass occlusion culling, which will be fixed in a future PR by using an HZB from the previous frame to get the initial list of occluders.Many, many improvements to be done in the future #11518, not least of which is code quality and speed. I don't even expect this to work on many types of input meshes. This is just a basic implementation/draft for collaboration.
Arguable how much we want to do in this PR, I'll leave that up to maintainers. I've erred on the side of "as basic as possible".
References: