Skip to content

Conversation

atlv24
Copy link
Contributor

@atlv24 atlv24 commented Sep 9, 2025

Objective

Solution

  • Add a StartMainPassPostProcessing to order against

Testing

  • bloom example

@atlv24 atlv24 added this to the 0.17 milestone Sep 9, 2025
@atlv24 atlv24 added A-Rendering Drawing game state to the screen P-Regression Functionality that used to work but no longer does. Add a test for this! S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 9, 2025
@atlv24 atlv24 added C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Sep 9, 2025
(Node3d::EndMainPass, NodePbr::VolumetricFog, Node3d::Bloom),
(
Node3d::EndMainPass,
NodePbr::VolumetricFog,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like maybe this should be ordered before EndMainPass if it's considered to be part of the main pass? I'm just not sure where it should go. I guess this works well enough though.

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 don't think its considered to be part of the main pass

Copy link
Contributor

Choose a reason for hiding this comment

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

I was 100% basing this based on the comment that was above 😅.

@JMS55
Copy link
Contributor

JMS55 commented Sep 9, 2025

Needs migration guide imo

@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 9, 2025
Copy link
Contributor

github-actions bot commented Sep 9, 2025

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@atlv24
Copy link
Contributor Author

atlv24 commented Sep 9, 2025

I dont believe this needs a migration guide, theres no breaking change here

@hukasu
Copy link
Contributor

hukasu commented Sep 9, 2025

Should the Bloom node be made to be after this new StartMainPassPostProcessing?

.add_render_graph_node::<ViewNodeRunner<BloomNode>>(Core3d, Node3d::Bloom)
.add_render_graph_edges(
Core3d,
(Node3d::EndMainPass, Node3d::Bloom, Node3d::Tonemapping),
)

@hukasu
Copy link
Contributor

hukasu commented Sep 9, 2025

Controvesial: all of the post processing nodes should be moved to a PostProcess3d on the bevy_post_processing

@atlv24
Copy link
Contributor Author

atlv24 commented Sep 9, 2025

the issue with moving it out is that you then need to depend on bevy_post_processing to order relative to post processing. we really just need a better more powerful render graph abstraction, id like to land this fix and figure that out next cycle

@atlv24 atlv24 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 Sep 9, 2025
@atlv24 atlv24 requested a review from IceSentry September 9, 2025 20:12
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

The ordering of volumetric fog does feel a bit weird since it's supposed to be at the start of post processing but is ordered before the new label. This is more of an issue with the render graph abstraction though.

@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 Sep 9, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 9, 2025
Merged via the queue into bevyengine:main with commit 369a760 Sep 9, 2025
36 checks passed
@github-project-automation github-project-automation bot moved this to Done in Rendering Sep 9, 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-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide P-Regression Functionality that used to work but no longer does. Add a test for this! 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: Done
Development

Successfully merging this pull request may close these issues.

InvalidNode(Bloom) crash if bevy_post_process is off
5 participants