Skip to content

Use RenderStartup for bevy_solari. #19918

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

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • Convert any resource mutations into systems (including resource init).
  • Add the render graph nodes from systems.
  • Create a SolariSystems system set that all systems belong to.
  • Check if the required features are present in a system and insert a resource if they are.
  • Add a run condition to SolariSystems to only execute systems if the resource is present.

Testing

  • Ran the solari example, and it still works.

@andriyDev
Copy link
Contributor Author

This PR is based off of #19912 so we don't have to restructure how we add render graph nodes. I will wait for that before sending it out for review.

@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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 2, 2025
@andriyDev andriyDev force-pushed the solari-conditional branch from 2b369a7 to a770f7d Compare July 2, 2025 17:41
@andriyDev andriyDev marked this pull request as ready for review July 2, 2025 17:45
@andriyDev andriyDev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 2, 2025
@andriyDev andriyDev requested review from atlv24, IceSentry and JMS55 July 2, 2025 17:45
@JMS55
Copy link
Contributor

JMS55 commented Jul 3, 2025

I really don't like this solution :/. It really complicates the solari code.

Perhaps we could make a generic version of this that multiple plugins could use?

@andriyDev
Copy link
Contributor Author

@JMS55 I'm not sure I agree that this complicates the solari code significantly. The only part that isn't specific to solari code is I think the fact that we have to configure the system sets 3 times.

I tried drafting this up and it's quite gross 0da4a59

In theory the conditional_render_set function would exist in bevy_render or something. But this leads to a lot of complexity that is just hidden. And I suspect it's going to be more difficult to debug because of these generic system sets.

To me, this seems like a lot of complexity that makes it less clear what's going on and why.


Looking through our code, here are these cases with finish impls that check some resources before inserting stuff.

  • crates\bevy_core_pipeline\src\oit\resolve\mod.rs
  • crates\bevy_pbr\src\atmosphere\mod.rs
  • crates\bevy_pbr\src\meshlet\mod.rs
  • crates\bevy_pbr\src\render\gpu_preprocess.rs
  • crates\bevy_pbr\src\render\mesh.rs
  • crates\bevy_pbr\src\ssao\mod.rs

Feel free to investigate, but the TL;DR for what these conditionals access is:

  • features from RenderDevice
  • limits from RenderDevice
  • "downlevel_capabilities" from RenderAdapter
  • allowed usages for textures in RenderAdapter
  • is_available from GpuPreprocessingSupport.

That's quite a mix of things we access, so it's not as simple as adding a run condition for features or something (plus I want to make the run conditions themselves as cheap as possible).

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-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants