Skip to content

Use RenderStartup for SSAO systems. #20206

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 4 commits into
base: main
Choose a base branch
from

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • Convert FromWorld impls to systems run in RenderStartup.
  • Create conditional_render_set to generalize the pattern of creating system sets + resources to conditionally run systems!

Thanks to @JMS55 for suggesting this generalization in #19918!

Testing

  • Ran the SSAO example and it still works!

@andriyDev andriyDev requested review from atlv24 and IceSentry July 19, 2025 22:18
@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-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 19, 2025
@andriyDev andriyDev force-pushed the startup-conditional branch from ebd1078 to 80ad27c Compare July 19, 2025 22:19
@andriyDev andriyDev requested a review from JMS55 July 19, 2025 22:20
@andriyDev andriyDev force-pushed the startup-conditional branch from 80ad27c to c8ebfa8 Compare July 19, 2025 22:33
@andriyDev andriyDev force-pushed the startup-conditional branch from c8ebfa8 to 3a91f1a Compare July 19, 2025 22:38
@andriyDev
Copy link
Contributor Author

I just noticed that there's quite a bad limitation to the conditional_render_set - there's no way to make sure the checking system runs after another. So if for example your condition depends on GpuPreprocessingSupport, you're kinda out of luck. I'm not sure how to fix this. We could create a generic system set like ConditionalRenderCheckSet that only applies to that checking system. Maybe with a bikeshed we could name that something more reasonable? I'm not sure.

@andriyDev
Copy link
Contributor Author

I added a little hack for this. If you want to order the condition, you can configure ConditionalRenderSetCondition (the system set) to run after its dependencies.

@andriyDev andriyDev force-pushed the startup-conditional branch from f552ac1 to 849bb1a Compare July 20, 2025 05:27
Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me, i think the conditional is still nice

@andriyDev andriyDev force-pushed the startup-conditional branch from 849bb1a to 7c5e973 Compare July 20, 2025 05:34
@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 and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 20, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This uses run conditions (not 100% ideal, conditionally adding systems / removing them would be slightly lower overhead), and there's gnarly "configure this system set across multiple schedules" nonsense, which is something that shouldn't be necessary.

That said, there's a lot to be said for having a consistent way to do this, and the exact details are clear and well documented.

Let me think about better patterns, to see if we can get something better now without waiting on further improvements to scheduling.

@alice-i-cecile alice-i-cecile added S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through and removed 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 20, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought harder about this, and I really don't like the complexity and ad-hoc nature of conditional_render_set. If we had commands.add_systems, would that work fine for you? That's one straightforward PR away. It's already possible by mutating Schedules, but a command would make it a bit clearer and less boilerplate.

The core request here is "conditionally add systems" and with the RenderStartup changes, we need to move this from plugin-init time to normal systems.

@andriyDev
Copy link
Contributor Author

andriyDev commented Jul 20, 2025

@alice-i-cecile I have a couple concerns with that.

First, it means we can't conditionally have systems in RenderStartup, because of course we can't add systems to the schedule we are currently in. That can be worked around by either making a mega system to build all the resources in one system (though some of these systems get pretty big already with a single resource), or you can check whether the feature is supported in the system.

Second, these systems won't show up on tools like bevy_mod_debugdump. It's a little concerning to me to have first-party "sneaky systems". Perhaps in the future, we'll have tools to debug schedules after launch, but that's probably not gonna be the first iteration anyway even for the bevy editor.

These are problems we could live with, but I wasn't excited about these caveats

@andriyDev andriyDev requested a review from alice-i-cecile July 20, 2025 21:48
@alice-i-cecile
Copy link
Member

Okay, hmm. I see your point there with the drawbacks of conditionally adding systems. If we're not going to conditionally add systems, a run condition is about as good as it can get.

This is well-encapsulated, so I can swallow it for now. Let me get feedback from the Rendering SMEs before taking on this tech debt though.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Needs-SME Decision or review from an SME is required and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through labels Jul 21, 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-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants