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

Remove Quat set methods in favour of constructors #45407

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

madmiraal
Copy link
Contributor

// FIXME: Quat is atomic, this should be done via construcror
//ADDFUNC1(QUAT, NIL, Quat, set_euler, VECTOR3, "euler", varray());
//ADDFUNC2(QUAT, NIL, Quat, set_axis_angle, VECTOR3, "axis", FLOAT, "angle", varray());

A Quat should be immutable. It should not have set_* methods. When required, a new Quat should be created using the equivalent constructor.

This PR removes the Quat set_* methods, and, where used, replaces them with the constructor equivalents. Note: This PR does not make Quat immutable. It's components are still public and they can still be (and are) changed within the engine.

For reference, deprecating them was originally suggested by @aaronfranke here as part of #16863.

core/math/quat.cpp Outdated Show resolved Hide resolved
core/variant/variant_call.cpp Show resolved Hide resolved
@vnen
Copy link
Member

vnen commented Jan 25, 2021

Please don't change GDNative before #44989 is merged, otherwise conflicts will haunt me. I'm removing those methods there anyway.

@akien-mga
Copy link
Member

Please don't change GDNative before #44989 is merged, otherwise conflicts will haunt me. I'm removing those methods there anyway.

That PR is now merged.

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.

Looks good to me (pending rebase for GDNative).

@madmiraal
Copy link
Contributor Author

Rebased.

@akien-mga akien-mga merged commit acd96e5 into godotengine:master Jan 26, 2021
@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