Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • Convert FromWorld impl to a system we run in RenderStartup.
  • Use a run condition on a system set to decide whether to run the OIT systems. This is effectively the proposal we will replicate for any other conditional system cases.

Testing

  • Ran the order_independent_transparency example and it works!

@andriyDev andriyDev changed the title Startup oit conditional Use RenderStartup for order independent transparency Jul 17, 2025
@andriyDev andriyDev requested review from atlv24 and IceSentry July 17, 2025 02:03
@andriyDev andriyDev added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 17, 2025

/// A resource to indicate that OIT resolve is supported.
#[derive(Resource)]
struct OitResolveSupported;
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@IceSentry IceSentry Jul 17, 2025

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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!

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

@nicopap nicopap left a 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.

@andriyDev andriyDev added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jul 19, 2025
@atlv24 atlv24 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants