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 property type of Sprite3D frame_coords to Vector2i from Vector2 #93982

Merged
merged 1 commit into from
Jul 7, 2024

Conversation

TokageItLab
Copy link
Member

The setter and getter types are correct, and there is probably no problem with compatibility since the only change is the type hint when binding.

@TokageItLab TokageItLab added bug topic:editor topic:animation cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jul 5, 2024
@TokageItLab TokageItLab added this to the 4.3 milestone Jul 5, 2024
@TokageItLab TokageItLab requested a review from a team July 5, 2024 20:23
@TokageItLab TokageItLab requested a review from a team as a code owner July 5, 2024 20:23
@TokageItLab TokageItLab added breaks compat and removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jul 5, 2024
@Mickeon
Copy link
Contributor

Mickeon commented Jul 5, 2024

The class reference file needs to also be changed (concerningly, none of the tests picked up on this)

@TokageItLab
Copy link
Member Author

TokageItLab commented Jul 5, 2024

@Mickeon Presumably, the class reference is generated by reference to the getter type and does not need to be fixed.

Both getters and setters had their type as Vector2i, but only PropertyInfo was incorrectly registered as Vector2, this PR fixes it.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

I'm fine with this personally. It breaks compatibility, sure, but it does as much as a bugfix would. Unsure it should be in 4.3, though.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

This doesn't break compat in C# because the generated bindings use the type of the getter/setter as the property type. So the C# property was already using the Vector2i type.

So regarding C# compat this change is fine 👍

@Mickeon
Copy link
Contributor

Mickeon commented Jul 6, 2024

That is actually shockingly fantastic, I think this is fine for 4.3 then

@akien-mga akien-mga merged commit 50373e6 into godotengine:master Jul 7, 2024
18 checks passed
@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.

Sprite3D Frame Coords bug
4 participants