-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Use RenderStartup
for order independent transparency
#20170
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
base: main
Are you sure you want to change the base?
Conversation
RenderStartup
for order independent transparency
|
||
/// A resource to indicate that OIT resolve is supported. | ||
#[derive(Resource)] | ||
struct OitResolveSupported; |
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 would prefer if this was called OitSupported
, same for the SystemSet
. The fact that everything has resolve in the name right now is more a side effect of the implementation but it shouldn't leak at that level because it will eventually do more than 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.
I looked at the code again and I misrepresented this a bit, but I still think this resource should be renamed.
I believe pretty much everything in OrderIndependentTransparencyPlugin
should be skipped if the check is false. I don't know why I didn't do it like this originally.
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.
To be clear, I think a refactor that move the check to that plugin should be separate, for this PR just keep Resolve out of the name.
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.
Done. Renamed the new types to omit the Resolve suffixes.
render_device: Res<RenderDevice>, | ||
render_adapter: Res<RenderAdapter>, | ||
) { | ||
if is_oit_supported(&render_adapter, &render_device, true) { |
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 might require a bit more work, but this function is only used in one other place, I think we could instead inline it here and pipe the resource to that other place that needs to know if OIT is supported.
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 would like to make this a followup rather than blocking this here. Minimal diff is better for these refactors IMO!
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.
Especially in rendering code it's better to avoid sprawling changes because everyone touches everything all the time. The other usage site (mesh_view_bindings.rs
) would require modifying more than one function, as you'd have to grab the OIT support state from the world and pass it around.
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.
Ah, right, I didn't realize it would be that complex, definitely follow up work then
e950cc7
to
df6766a
Compare
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 just a really good changeset in line with the goal of minimizing the use of finish()
.
I also do like the code.
Objective
Solution
Testing
order_independent_transparency
example and it works!