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

Rename 2D NoiseTexture to NoiseTexture2D #64864

Merged
merged 1 commit into from
Aug 26, 2022

Conversation

aaronfranke
Copy link
Member

This rename would be helpful to have done before implementing godotengine/godot-proposals#4946

However, I think this stands on its own. Since it inherits Texture2D it makes sense to have the name end in Texture2D, so I'm opening a PR just for this. I briefly discussed this on RocketChat.

@aaronfranke aaronfranke added this to the 4.0 milestone Aug 25, 2022
@aaronfranke aaronfranke requested a review from a team as a code owner August 25, 2022 01:00
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.

Discussed on Rocketchat, we should merge this before beta so that we have room to implement a NoiseTexture3D in the future without breaking compatibility.

@akien-mga
Copy link
Member

What about all the other *Texture classes though, like AnimatedTexture or CanvasTexture?

@aaronfranke
Copy link
Member Author

aaronfranke commented Aug 25, 2022

@akien-mga I don't know about each case, but in general that makes sense and I would be in favor of renaming some of those too. For this PR, in addition to the Texture2D inheritance, I am focused on this case because 3D noise is known to be a desired feature so if those cases deserve a rename then this case definitely does with this additional reason in mind.

@akien-mga
Copy link
Member

Discussed it with @reduz who seems to be fine with the change:

<Akien> @reduz Thoughts on #64864 ? My main doubt is that it adds more inconsistency in the naming of our texture classes.
Currently, most classes inheriting Texture2D don't use the 2D suffix:
[img of the classes inheriting Texture2D]
<reduz> @Akien it would make sense if we think a NoiseTexture3D makes sense
which may do
the others are clearly 2D only
<Akien> Yeah this PR is in support of a proposal to add NoiseTexture3D
My doubt is whether we'll ever want to also add AnimatedTexture3D or something like that.
<reduz> probably not, and that reminds me that i need to remove ProxyTexture
its not so useful now and its not so efficient in Vulkan 

@akien-mga akien-mga merged commit c54125d into godotengine:master Aug 26, 2022
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the noise-texture-2d branch August 26, 2022 14:47
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.

4 participants