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 deprecated mesh conversion #68428

Merged
merged 1 commit into from
Nov 11, 2022

Conversation

nikitalita
Copy link
Contributor

In #60309, when octahedral mesh conversion was added to Godot 4.x, the src_offset increment were modified erroneously and changed the increment value.

https://github.com/godotengine/godot/pull/60309/files#r1017101020
https://github.com/godotengine/godot/pull/60309/files#r1017104192

This restores the previous offset values and adds clarifying comments where necessary.

@nikitalita
Copy link
Contributor Author

re: @clayjohn @The-O-King

@Calinou Calinou added this to the 4.0 milestone Nov 8, 2022
@clayjohn clayjohn self-requested a review November 8, 2022 22:33
@fire fire requested a review from a team November 9, 2022 21:34
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, only non-compressed non-octahedral tangents needs to be updated.

I left a comment on each line to show which entry from 3.x it matches for future reference.

Once the non-compressed non-octahedral tangent code is fixed this should be good to merge

scene/resources/mesh.cpp Show resolved Hide resolved
scene/resources/mesh.cpp Show resolved Hide resolved
scene/resources/mesh.cpp Show resolved Hide resolved
scene/resources/mesh.cpp Show resolved Hide resolved
scene/resources/mesh.cpp Show resolved Hide resolved
scene/resources/mesh.cpp Outdated Show resolved Hide resolved
scene/resources/mesh.cpp Outdated Show resolved Hide resolved
@clayjohn
Copy link
Member

The 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

@nikitalita
Copy link
Contributor Author

I'm not going to be near a computer for the next week. Can you please squash and merge?

@clayjohn
Copy link
Member

I'm not going to be near a computer for the next week. Can you please squash and merge?

Sorry, no. We don't have that enabled for this repository :(

@nikitalita
Copy link
Contributor Author

apparently GitHub codespaces is free now and works on ipad safari, so there you go.

@akien-mga akien-mga merged commit 3263970 into godotengine:master Nov 11, 2022
@akien-mga
Copy link
Member

Thanks!

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