Skip to content

Use RenderStartup for bevy_solari.#19918

Closed
andriyDev wants to merge 1 commit intobevyengine:mainfrom
andriyDev:solari-conditional
Closed

Use RenderStartup for bevy_solari.#19918
andriyDev wants to merge 1 commit intobevyengine:mainfrom
andriyDev:solari-conditional

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 IceSentry, JMS55 and atlv24 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).

@IceSentry
Copy link
Contributor

IceSentry commented Jul 13, 2025

To me this doesn't look particularly that complicated. It's a bit more setup code, but it also makes things more explicit which I think is valuable. I like the idea of a HasRequiredFeatures resource for a run condition. With that said, I'm not entirely sure the system sets is strictly needed? It's only used in a few places and it only exists for the purpose of the run condition but I feel like adding the run condition directly where it's needed might be more explicit and lead to overall a bit less code? We can always add back the system set in the future if we need more complex scheduling.

/// are enabled.
///
/// Now systems can do a cheap check for if the resource exists.
fn check_solari_has_required_features(mut commands: Commands, render_device: Res<RenderDevice>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused where is this system actually added to RenderStartup?

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 totally goofed haha. I added the system now!

@andriyDev andriyDev force-pushed the solari-conditional branch from a770f7d to fcdc082 Compare July 13, 2025 16:55
@andriyDev
Copy link
Contributor Author

Due to the refactors from SolariPlugin to a plugin group, I made a new plugin just focused on the configure_sets stuff, and added that to the plugin group.

@andriyDev andriyDev closed this Mar 5, 2026
@andriyDev andriyDev deleted the solari-conditional branch March 5, 2026 01:39
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

No open projects
Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants