Handle unallocated GPU memory for mesh extraction #20112
Conversation
| None => (0, 0), | ||
| None => { | ||
| // GPU memory for this mesh hasn't been allocated yet. Retry next frame. | ||
| meshes_to_reextract_next_frame.insert(entity); |
There was a problem hiding this comment.
If this is the fix, we might need to do the same for the mesh_index_slice a few lines below this.
I'm unsure why it previously didn't return None in this case. I feel like this might not be the full fix. I'll need to investigate further.
There was a problem hiding this comment.
Oh, thinking about this more, no, we shouldn't do this for the index_slice because not having indices is a valid case
There was a problem hiding this comment.
Is there a case where vertex_slice is ready, but not index_slice?
There was a problem hiding this comment.
For non indexed meshes index_slice will never be ready. I'm not sure it's possible for an indexed mesh to not have the index_slice ready. AFAIK they are allocated together but I could be wrong.
| @@ -1098,7 +1098,11 @@ impl RenderMeshInstanceGpuBuilder { | |||
| mesh_vertex_slice.range.start, | |||
| mesh_vertex_slice.range.end - mesh_vertex_slice.range.start, | |||
| ), | |||
| None => (0, 0), | |||
| None => { | |||
| // GPU memory for this mesh hasn't been allocated yet. Retry next frame. | |||
There was a problem hiding this comment.
What cases will this occur?
We don't want to end up in an infinite retry loop for failed meshes.
There was a problem hiding this comment.
I'm not sure there's a way to detect this here? With that said it's also possible this reextract thing should happen somewhere else and not here.
There was a problem hiding this comment.
I really don't like that this is an Option: a Result would have a lot clearer semantics to me.
There was a problem hiding this comment.
I'm still a bit worried as to why the original code didn't consider this an invalid case and exit early, but this looks correct to me.
Also it feels a bit weird that this function is the one that modifies meshes_to_reextract_next_frame. I feel like it should be the job of the parent to decide what to do when a None is returned but that's out of scope for this PR and I don't know if this should change.
janhohenheim
added
C-Bug
|
@superdump I'm not confident on this PR, so I'm going to skip it on my merge train. It feels like this is a good bug fix inside of messy code, and I don't have the rendering expertise to judge whether we should merge it as is or block it on a more comprehensive cleanup. |
|
Based on the feedback I got so far it seems we should block the change for a more comprehensive cleanup, I support this decision to skip for the merge train now. |
alice-i-cecile
added
S-Waiting-on-Author
Objective
Fix
examples/asset/alter_meshwhere the right-hand mesh disappeared and the cube flickered when swapping between cube and sphere at runtime.Fixes #18967
Solution
Early-exit
RenderMeshInstanceGpuBuilder::updatewhen the mesh’s GPU slice is not yet allocated and queue the entity for re-extraction on the next frame.Testing
Ran
cargo run --example alter_meshon macOS/Metal, toggling meshes (Space) and vertex scaling (Return) for several minutes; verified both meshes remain visible, no flicker occurs, and performance is unchanged.Showcase