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

Properly set sky_cover property of sky_material to Nil in setter #75469

Merged

Conversation

ibotha
Copy link
Contributor

@ibotha ibotha commented Mar 29, 2023

issue 74259 seems to be caused by being the value being set to "RID()" in the setter for the sky_cover property. I was able to fix this by setting the sky cover property back to Variant::Nil when it is reset in the editor.

I have tested this locally by running the engine before the fix, seeing the bug is present, then running it after the fix and seeing it works as expected.

Bugsquad edit:

@ibotha
Copy link
Contributor Author

ibotha commented Mar 29, 2023

My understanding of the engine is limited. If it is meant to be the case that RID() behaves as a nil value then I can make that change too

@Calinou Calinou added this to the 4.1 milestone Mar 29, 2023
@ibotha ibotha force-pushed the fix/74259-sky-cover-not-reset-properly branch from 48c75c2 to 45a7d3d Compare March 29, 2023 18:52
@ibotha
Copy link
Contributor Author

ibotha commented Mar 29, 2023

Noticed the formatting caused one of the tests to fail so I fixed it

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! This pattern is used in a few other places in the codebase, so we will need to keep our eyes open to other areas where this might cause an issue

@clayjohn
Copy link
Member

clayjohn commented May 3, 2023

After looking into this a bit more, the same change is needed for PanoramaSkyMaterial and for PhysicalSkyMaterial. It would be nice to make those changes in this PR so they are all grouped together

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

See clayjohn's comment.

@ibotha
Copy link
Contributor Author

ibotha commented May 16, 2023

Alright, I have updated the other sky materials, sorry it took a while for me to get back to this.

@clayjohn
Copy link
Member

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

@ibotha ibotha force-pushed the fix/74259-sky-cover-not-reset-properly branch from 69c6873 to 74041e3 Compare May 21, 2023 17:36
@ibotha
Copy link
Contributor Author

ibotha commented May 21, 2023

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

Alright, The commit has been squashed

@akien-mga akien-mga merged commit 65fa775 into godotengine:master May 22, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants