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

Fix compatibility breakage from adding NoiseTexture3D #76557

Merged
merged 1 commit into from
May 3, 2023

Conversation

clayjohn
Copy link
Member

Follow up to #76486

#76486 broke compatibility by introducing a new parameter to Noise.get_image() and Noise.get_seamless_image(). The new parameter was called p_depth even though the function did not return a 3D image (i.e. an array of Images), so it really represented a z offset.

This PR introduces a get_image_3d() for use with 3D textures and it abstracts the old get_image() into an internal function _get_image() which handles both 2D and 3D.

This PR also:

  1. Fixes a case of calling wait_on_thread on a thread that hasn't been started
  2. Optimizes the generation of normalized noise textures (the speedup is about 33% even for regular 2D textures, it is significantly more for 3D)

Finally, I have left the seamless generation algorithm the same. Although I suspect it can be generalized to the 3D case a bit more and optimized.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Didn't do an in depth review, but the overall changes seem sensible, and undoing the compat breakage is great.

@Lasuch69
Copy link
Contributor

Lasuch69 commented Apr 28, 2023

This looks awesome! I wanted to change as little as possible in the current implementation, and now I see that it was unnecessary.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally on the MRP linked in #76486, it works.

Should be good to merge after applying suggestions.

@clayjohn
Copy link
Member Author

Should be good to merge after applying suggestions.

Done!

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Looks great! Unfortunately I haven't had the time to review the original PR, but this definitely looks like a sensible fixup/cleanup. Tested it and it looks like everything is still working fine.

modules/noise/noise.h Outdated Show resolved Hide resolved
Also optimize some of the Noise methods
@Zireael07
Copy link
Contributor

note that "Depth" and the like are common names for parameters involving z axis

@mhilbrunner mhilbrunner merged commit c14f870 into godotengine:master May 3, 2023
@mhilbrunner
Copy link
Member

Thanks for fixing this up! 🎉

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.

7 participants