Skip to content

Add SkipDeferredLighting component #19628

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

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Jun 13, 2025

Adds a new component for when you want to run the deferred gbuffer prepass, but not the lighting pass.

This will be used by bevy_solari in the future, as it'll do it's own shading pass, but still wants the gbuffer.

@JMS55 JMS55 added the A-Rendering Drawing game state to the screen label Jun 13, 2025
@DGriffin91
Copy link
Contributor

DGriffin91 commented Jun 13, 2025

This option exists specifically for this use case:

pub add_default_deferred_lighting_plugin: bool,

Is it not sufficient?

@JMS55
Copy link
Contributor Author

JMS55 commented Jun 13, 2025

No because that's global, instead of per-view :/

@DGriffin91
Copy link
Contributor

Ahh, gotcha. That make sense. Wonder if this option should be removed then in favor of the per view version?

@JMS55
Copy link
Contributor Author

JMS55 commented Jun 13, 2025

Lets leave it for now imo. I think there's still benefit in the global option.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 13, 2025
@JMS55 JMS55 requested a review from Elabajaba June 13, 2025 20:12
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This makes sense in the short term.

I think its worth trying to think about how we could make this sort of thing a product of the configured pipeline / camera, rather than making it per-entity user-app-world configuration. It would be nice if we could spawn a normal gltf into a context that raytraces it, rather than needing to modify the gltf scene to fit the inner workings of the renderer's requirements. SkipDeferredLighting is an implementation detail that we could (and I will argue should) abstract away from the user app world.

Another path (if we decide "raytracing" is a scene property and not a camera/pipeline property) would be to scope this to being a material property (ex: materials that are configured to be "raytraceable" would have this property enabled). That way, it would be as simple as patching all of the materials in the scene to be "raytraced materials".

In the context of rendering, I like to think of the user's app world components as the "user facing contract" for an entity. SkipDeferredLighting doesn't feel like a user facing contract for entities. It feels like an implementation detail of a render pipeline.

@JMS55
Copy link
Contributor Author

JMS55 commented Jun 13, 2025

I changed the PR a little, now it's no longer inserted in the main world and extracted. I'll have bevy_solari insert it directly into the render world when extracting SolariLighting.

@cart
Copy link
Member

cart commented Jun 13, 2025

I erroneously thought this was intended to be a per-entity component (sorry for skimming this ... that was irresponsible of me). I actually do not strongly object to using Components like this in the context of the Camera API + required components, as this sort of "view pipeline configuration" on Cameras is par for the course there.

This is still "implementation detail-ey" and theres still an argument to abstract this out from the user (ex: by doing this in the render world). But I care much less about that case. Feel free to do what you think is best there.

@alice-i-cecile alice-i-cecile 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 Jun 13, 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-Usability A targeted quality-of-life change that makes Bevy easier to use 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