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

Keep Variant type after zero() #84597

Merged
merged 1 commit into from
Nov 8, 2023
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 7, 2023

Fixes godotengine/godot-demo-projects#979
Regression from #80813

Variant.zero() (apparently used by animation) is supposed to revert the value to default. However in case of some types, it clears internals and changes the type to NIL. This PR ensures that the type is retained and initializes the value anew. (tested only with Transform3D)

@KoBeWi KoBeWi added this to the 4.3 milestone Nov 7, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner November 7, 2023 23:03
this->clear();
if (type != prev_type) {
// clear() changes type to NIL, so it needs to be restored.
Copy link
Member

Choose a reason for hiding this comment

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

Might be slighter better to add a bool reset_type = true to clear() instead of doing it like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

_clear_internal() called by clear() will free the internal data, so it needs to be re-created.
The best solution would be probably reworking zero() to not use clear().

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 fairly safe, zero() is only used in a few paths:

$ rg -g'!thirdparty' "[\.\t]zero\("
scene/animation/animation_mixer.cpp
661:                                            track_value->init_value.zero();

core/math/basis.h
187:            rows[0].zero();
188:            rows[1].zero();
189:            rows[2].zero();

servers/physics_3d/godot_body_3d.cpp
71:                             center_of_mass_local.zero();

center_of_mass_local is Vector2, and rows in Basis are real_t, so neither will be affected.

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

I was trying to get the same result by different ways of writing it, but this seems to be the simplest and better way. Also I applied this to the Array Blending I am working on and it works well.

@akien-mga akien-mga merged commit ae20b74 into godotengine:master Nov 8, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke
Copy link
Member

See also #59186. Since this method is supposed to reset to the default value, calling it "zero" doesn't seem correct.

@KoBeWi KoBeWi deleted the zeroed_existence branch November 8, 2023 19:00
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.

Gui in 3D Demoproject no longer works
5 participants