Skip to content
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

Add constraint to render graph #37

Merged
merged 1 commit into from
Oct 26, 2024
Merged

Add constraint to render graph #37

merged 1 commit into from
Oct 26, 2024

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Oct 25, 2024

Fixes #34

I am not sure about #33.

NodeUi::UiPass's ordering constraints seem nonsensical to me. I am probably not understanding it correctly. But I think that as long as we do your stuff before Node2d:: EndMainPassPostProcessing, then this should also fix #33. I'm having trouble actually reproducing the issue though before or after this changeset though.

Not a rendering expert, so this should be scrutinized.

Following the render graph setup in custom_post_processing, it seems that our render graph nodes are constrained to run after Node2d::EndMainPass, but otherwise free to run whenever. Might be before or after any of:

Node2d::Bloom
Node2d::Tonemapping
Node2d::Fxaa
Node2d::Smaa
Node2d::Upscaling
Node2d::ContrastAdaptiveSharpening
Node2d::EndMainPassPostProcessing

I'm not totally sure this is exactly where the nodes belong, but it seems to fix the issue on my end.

I did the following experiments:

(Node2d::Upscaling, SdfPass, LightMapPass, LightingPass) // Seems to fail 100% of the time
(Node2d::Smaa, SdfPass, LightMapPass, LightingPass, Node2d::Upscaling) // Seems to work all the time

So it seems like this definitely needs to run before Node2d::Upscaling.

@rparrett
Copy link
Contributor Author

Finished looking into #33 about as much as I want to and updated the description.

Copy link
Owner

@jgayfer jgayfer left a comment

Choose a reason for hiding this comment

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

Awesome work. I agree that this is very likely what's been causing the issue. Without referring back to Node2d, the final graph order was very likely not deterministic.

I can reproduce the bug on main pretty easily, and I didn't see it after running this branch 15-20 times. I'm happy to call this The Fix.

Are you able to update CHANGELOG.md with this fix under the unreleased header, referring to this PR? (I should add this to a PR template).

Something like:

### Fixed

-  Non-deterministic render graph, occasionally causing lights to not render and/or affect elements in unintended order (#37).

Copy link
Owner

@jgayfer jgayfer left a comment

Choose a reason for hiding this comment

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

You know what, all good, I can add that changelog entry!

@jgayfer jgayfer merged commit b185a33 into jgayfer:main Oct 26, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lights *sometimes* not working at all Ambient light *sometimes* affect Bevy UI Texts
2 participants