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

Use defaults to initialize sky data in case of no sky #79812

Merged

Conversation

ParsleighScumble
Copy link
Contributor

@ParsleighScumble ParsleighScumble commented Jul 23, 2023

Fixes #79509. Effectively makes parameter "sky" optional - we only check it where it's used and everywhere it's not needed is not behind a condition anymore. Classic fog now no longer results in TDR when clear color is used for sky.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good to me! The same changes are needed in GLES3 as well before this is ready to merge:

void RasterizerSceneGLES3::_setup_sky(const RenderDataGLES3 *p_render_data, const PagedArray<RID> &p_lights, const Projection &p_projection, const Transform3D &p_transform, const Size2i p_screen_size) {

@ParsleighScumble
Copy link
Contributor Author

No problem, I'll have that done in a jiffy.

@ParsleighScumble
Copy link
Contributor Author

One thing - I saw affect_sky fog property doesn't do anything to the clear color in GLES3. Should I fix that before we merge this?

@clayjohn
Copy link
Member

One thing - I saw affect_sky fog property doesn't do anything to the clear color in GLES3. Should I fix that before we merge this?

If you want to, then sure! But IMO that would be best done in a follow-up PR.

@clayjohn
Copy link
Member

The updated code looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

@ParsleighScumble ParsleighScumble force-pushed the parsleigh/init-sky-scene-state branch from e44c126 to 67c13fe Compare July 24, 2023 17:42
@ParsleighScumble
Copy link
Contributor Author

Should be good to go.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I've tested locally and I didn't identify any regressions. I wasn't able to reproduce the reported bug though, so I can't confirm that it is actually fixed.

At any rate, I am happy to move forward with merging this

@clayjohn clayjohn modified the milestones: 4.x, 4.2 Jul 25, 2023
@YuriSizov YuriSizov merged commit 08cffc1 into godotengine:master Jul 25, 2023
@YuriSizov
Copy link
Contributor

Thanks! And congrats on your first merged Godot PR!

@ParsleighScumble
Copy link
Contributor Author

Thanks, Yuri!

@ParsleighScumble ParsleighScumble deleted the parsleigh/init-sky-scene-state branch July 27, 2023 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using Fog Sun Scatter property on Windows 10 (QEMU) freezes system
5 participants