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

Rename VariantZeroAssigner to VariantDefaultInitializer #59186

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

aaronfranke
Copy link
Member

Discussed on RocketChat. VariantZeroAssigner's name indicates that it should assign zero, but it actually assigns the default value. For some types, such as Basis, Transforms, Color, Quat, the default value is non-zero. The new name VariantDefaultInitializer was suggested by @akien-mga.

definit

@TokageItLab
Copy link
Member

TokageItLab commented Mar 29, 2022

I am wondering if this PR will make Variant::zero() obsolete. To begin with, there are few parts that use Variant::zero(), and it is only needed for blending ValueTracks in AnimationTree. So if this PR is approved, perhaps the right way to go would be to obsolete Variant::zero() and implement AnimationTree::set_variant_to_zero() as a local function of AnimationTree.
Context: #59474 and #59490

};

template <>
struct VariantZeroAssigner<bool> {
struct VariantDefaultInitializer<bool> {
static _FORCE_INLINE_ void zero(Variant *v) { *VariantInternal::get_bool(v) = false; }
Copy link
Member

Choose a reason for hiding this comment

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

If this is approved, then zero should also be renamed everywhere. But let's see what @reduz thinks first.

@akien-mga
Copy link
Member

Discussed in PR review meeting, @reduz (and maybe @vnen) need to check a bit deeper why this is needed and why it doesn't reset all types of Variants (e.g. String, Callable).

@Iniquitatis
Copy link
Contributor

Don't know how much of a compatibility breakage it will cause (if any), but can the zero method be renamed accordingly? IIRC, default is a reserved keyword, so what about default_value, for example?
As of now, it looks a bit weird that DefaultInitializer implenents the zero method, which doesn't zero out a value in all cases.

@vnen
Copy link
Member

vnen commented Dec 4, 2023

The change makes sense to me. Since it's an internal change it won't break compatibility. I do agree that the zero() methods should also be renamed.

@AThousandShips
Copy link
Member

I'd just call it init, it is an Initializer and the other Initializers call it that

@akien-mga akien-mga removed this from the 4.x milestone Dec 5, 2023
@akien-mga akien-mga added this to the 4.3 milestone Dec 5, 2023
And the method zero() is now init()
@YuriSizov YuriSizov merged commit ee9ff6a into godotengine:master Dec 18, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@aaronfranke aaronfranke deleted the vardefinit branch December 18, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Third Option
Development

Successfully merging this pull request may close these issues.

7 participants