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

Implemented texture_2D_layered_initialize #77224

Conversation

patrickdown
Copy link
Contributor

This change implements functionality for texture_2D_layered_initialize that was previously stubbed for OpenGL.

@patrickdown patrickdown requested a review from a team as a code owner May 19, 2023 00:30
@BastiaanOlij BastiaanOlij added this to the 4.1 milestone May 19, 2023
@BastiaanOlij BastiaanOlij requested a review from clayjohn May 19, 2023 00:36
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

This looks good to me, @clayjohn might want to give a second opinion in case we've missing something but I think this is correct.

@patrickdown patrickdown force-pushed the implement_texture_2d_layered_initialize branch 2 times, most recently from 20910b7 to 7c71164 Compare May 19, 2023 01:19
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.

This looks mostly good, but it should include a bit more validation like what is done in the RD implementation of this function:
https://github.com/godotengine/godot/blob/master/servers/rendering/renderer_rd/storage_rd/texture_storage.cpp#L859

Particularly, this is missing support for when the texture type is TEXTURE_LAYERED_CUBEMAP

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 mostly good. I just made a small suggestion for the error

@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

@patrickdown patrickdown force-pushed the implement_texture_2d_layered_initialize branch from f6b1dae to 391a1bf Compare May 27, 2023 18:07
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.

Confirming @clayjohn's implicit approval now that it has been squashed.

@akien-mga akien-mga merged commit f2e91ea into godotengine:master May 29, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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