-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Atmosphere generated environment map lighting #20529
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
Conversation
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet! thanks for splitting it up :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think user-facing docs need some work still. They focus too much on technical details (renders a cubemap), rather than what they're useful for (getting the atmosphere to contribute lighting).
Code looks good though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shaders look great! Just a couple nits for stuff on the rust side, but nothing blocking :)
@@ -139,6 +148,14 @@ impl Plugin for AtmospherePlugin { | |||
configure_camera_depth_usages.in_set(RenderSystems::ManageViews), | |||
queue_render_sky_pipelines.in_set(RenderSystems::Queue), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this system should match prepare_atmosphere_probe_pipelines
naming-scheme wise, and be in the same set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense but should this be the other way around? i see auto exposure also uses the Queue
render set, and queue_
naming prefix. but it's a view specific pipeline. I almost think I should match my naming to this since the entity we are considering in this case for queueing is the view / camera.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as they match I don't mind tbh
Thanks for everyone’s feedback! I have addressed every comment, let me know if this version still needs any fine tuning. It is ready for a final review! |
prepare_atmosphere_probe_bind_groups.in_set(RenderSystems::PrepareBindGroups), | ||
queue_atmosphere_probe_pipelines | ||
.in_set(RenderSystems::Queue) | ||
.after(init_atmosphere_probe_layout), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.after(init_atmosphere_probe_layout), |
This is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mate-h It seems this comment was missed. Is this line redundant or we decided to leave it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in previous versions I was getting a runtime error without explicitly ordering systems, but with the queue render set changes it is probably safe to remove it! I have not had a chance to test without this line. Feel free to raise a PR if you do so! Thanks.
@mate-h needs release notes, no? |
I am not at home right now, but i can do it as a follow up tomorrow! The release note is super simple though. Just added this new component no breaking changes to the atmosphere. |
Yeah, but we should highlight it in the blog post :) |
Objective
Solution
Testing
Showcase
Limitations, out of scope