-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 software raster + start of cleanup #14623
Conversation
…iting material ID during raster
var max_x = u32(ceil(max3(vertex_0.x, vertex_1.x, vertex_2.x))); | ||
var max_y = u32(ceil(max3(vertex_0.y, vertex_1.y, vertex_2.y))); | ||
max_x = min(max_x, u32(view.viewport.z) - 1u); | ||
max_y = min(max_y, u32(view.viewport.w) - 1u); |
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 also need to min_x = max(0, min_x)
etc?
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.
Iirc the u32(foo) should turn all negatives into 0.
// Scanline setup | ||
let edge_012 = -w_x; | ||
let open_edge = edge_012 < vec3(0.0); | ||
let inverse_edge_012 = select(1.0 / edge_012, vec3(1e8), edge_012 == vec3(0.0)); |
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 1e8
?
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.
Idk, it's what the nanite slides 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.
I tried to understand what this is doing, but never quite figured it out.
crates/bevy_pbr/src/meshlet/visibility_buffer_software_raster.wgsl
Outdated
Show resolved
Hide resolved
|
||
// Iterate scanline X interval | ||
for (var x = x0; x <= x1; x++) { | ||
// Check if point at pixel is within triangle (TODO: this shouldn't be needed, but there's bugs without it) |
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.
that's annoying, i'll see if i can figure this out but dont block merge 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.
Might just be needed on the first and last pixel (x0, x1), and can be skipped for [x0 + 1, x1 - 1]
@@ -294,6 +289,7 @@ fn simplify_meshlet_groups( | |||
let target_error = target_error_relative * mesh_scale; | |||
|
|||
// Simplify the group to ~50% triangle count | |||
// TODO: Simplify using vertex attributes |
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.
Is this about the bevy mesh api or more specific to meshlets?
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.
Specific to meshlets. Simplification (i.e. the LOD building) only accounts for position atm, and not things like UVs/normals. See zeux/meshoptimizer#158.
|
||
/// Manages data for each entity with a [`MeshletMesh`]. | ||
#[derive(Resource)] | ||
pub struct InstanceManager { |
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 it's fine with the name space, but that feels a bit generic? Why not something like VirtualGeometryMeshManager
? To be clear, this isn't a blocker or anything, I'm just curious.
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.
Eh I didn't want to get hung up on naming. We can change it later.
|
||
/// Manages uploading [`MeshletMesh`] asset data to the GPU. | ||
#[derive(Resource)] | ||
pub struct MeshletMeshManager { |
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 this should have Gpu somewhere in the name to clarify it's puprose?
&BindGroupLayoutEntries::sequential( | ||
ShaderStages::COMPUTE, | ||
( | ||
storage_buffer_read_only_sized(false, None), |
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.
That's a lot of bindings, but that does feel a lot easier to read then when we were still using the raw structs 😅
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.
Yeahhh I want automated bind groups in the render graph for a reason... Arguably this explicitness is better for perf, but it can't be that much slower to hash-and-cache, and this sucks to write and tweak.
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.
Okay, I'm not seeing anything obviously wrong. I confirmed that the code doesn't break anything else. The various *Manager
structs are a bit generically named, but that doesn't really matter for now.
I get a clippy lint locally but it's in a crate that isn't modified in this PR so not sure what's up with that. Other than that LGTM.
This PR maybe made things faster, but it also greatly reduces the range of hardware where it works. meshlets used to work on macOS or on Vulkan with software renderer, not anymore. I don't know about DX12 but there's a comment saying it's now Vulkan only with a recent GPU. Is it worth it making it faster if it won't run anymore on hardware where it would be useful for it to be faster? |
I'm not open to supporting GPUs without 64bit atomics (meaning no software raster). Besides being an insane amount of extra code to support (pretty much an entire extra copy of the codebase), there's no point in telling artists to design their scenes around this feature for pixel level geometry detail, and then have older users try to use it, completely fall over due to lack of software raster + low vram, and then blame the game developers for not optimizing their game when the developers can't do anything besides making an entire second copy of the scene designed for working without meshlets. This is not a feature meant for older platforms, and the only reason it ran before was a temporary setup so that I could develop the rest of the feature without waiting for wgpu to implement 64bit atomics. It should run on DX12 (SM 6.6+ iirc) and Metal (M2+ iirc), except wgpu/naga is bugged on DX12/Metal with these shaders. |
For my future reference, another good SW raster tutorial: https://web.archive.org/web/20050408192410/http://sw-shader.sourceforge.net/rasterizer.html |
Objective
Solution
Misc changes
Migration Guide