-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[wgpu-core] ray tracing follow-up #6552
Conversation
Ray Tracing tests passed on my computer. |
Demoting to draft while dependents get worked on. |
@cwfitzgerald: But...that PR has already been pretty thoroughly reviewed, it's just waiting on @jimblandy to finish a minor change so that it can merge. It feels like this PR really is |
We should probably unify policy on this at some point, from my perspective there was a bunch of stuff going on in the base PR, I have no idea how complete or what change sets actually matter (is this PR even up to date with the current state of the dependency). Which is not really reviewable without extra instructions. |
I've started reviewing 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.
Reviewing commit by commit. "[wgpu-core] ray tracing: use error handling helpers" looks great. Proceeding to the next.
self.raw | ||
.get(guard) | ||
.map(|raw| raw.as_ref()) | ||
.ok_or_else(|| DestroyedResourceError(self.error_ident())) |
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.
What you wrote is completely fine, but I wonder if you think the below is slightly quicker to read:
self.raw | |
.get(guard) | |
.map(|raw| raw.as_ref()) | |
.ok_or_else(|| DestroyedResourceError(self.error_ident())) | |
let Some(tlas) = self.raw.get(guard) else { | |
return Err(DestroyedResourceError(self.error_ident())); | |
}; | |
Ok(tlas.as_ref()) |
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'm impartial to changing this but I copied it over from one of the other try_raw
methods. I think if we want to change it we should change the others as well.
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 second commit, "[wgpu-core] ray tracing: remove acceleration structures from pending writes", is fine.
Third commit is fine:
|
Fourth commit is fine. |
fifth commit is delicious |
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, this looks great. I had two suggestions, please see above.
…writes The acceleration structures are already being kept alive by the tracker in the command encoder.
…coderInFlight`, remove it from `ActiveSubmission` Use `CommandBufferMutable.temp_resources` to store the ray tracing related staging buffers and scratch buffers. These shouldn't have been stored in `PendingWrites.temp_resources` because they can get destroyed early (before the command buffer that uses them has been submitted) if any other command buffer gets submitted first.
`Tlas.destroy` didn't check if the `Tlas` is used in a bind group of an active submission. The only reason we need the `destroy` methods for textures and buffers is because they allow users to eagerly release memory in browser implementations. I think we can remove the destroy methods on the acceleration structures for now as they complicate the picture without any gain. If they will be needed for Firefox we can add them back.
Follow-up to #6291.
Depends on #6544.