Delete VariantGetInternalPtr and VariantImplicitConvert, replace with VariantInternalAccessor
#105254
Conversation
…mplicitly converted to and from `Variant`. De-duplicate a lot of `VariantGetInternalPtr`, `VariantInternalAccessor`, `VariantInitializer` and `VariantDefaultInitializer`.
Replace uses with `VariantInternalAccessor`.
Ivorforce
changed the title
VariantGetInternalPtr and VariantImplicitConvert, replace with VariantInternalAccessorVariantGetInternalPtr and VariantImplicitConvert, replace with VariantInternalAccessor
There was a problem hiding this comment.
Tested locally, it works as expected. Code looks good to me.
However, this change appears to make the binary somewhat larger. Example with a stripped x86_64 Linux editor (production=yes lto=full):
master: 131,325,296 bytesThis PR: 132,642,576 bytes (+ 1.32 MB)
Edit: Disregard the above paragraph. I ran clean builds again and binary size is roughly identical (less than 1 KB difference).
| add_constructor<VariantConstructor<Transform2D, float, Vector2>>(sarray("rotation", "position")); | ||
| add_constructor<VariantConstructor<Transform2D, float, Size2, float, Vector2>>(sarray("rotation", "scale", "skew", "position")); | ||
| add_constructor<VariantConstructor<Transform2D, double, Vector2>>(sarray("rotation", "position")); | ||
| add_constructor<VariantConstructor<Transform2D, double, Size2, double, Vector2>>(sarray("rotation", "scale", "skew", "position")); |
There was a problem hiding this comment.
Should these be real_t? That's what's used by the actual Transform2D constructors
There was a problem hiding this comment.
I think it rejected the float constructor because there's no Variant holding float. It would likewise reject real_t. It's been a while since I've looked at this code though.
| VariantInitializer<float>::init(&value); | ||
| VariantDefaultInitializer<float>::init(&value); | ||
| VariantInitializer<double>::init(&value); | ||
| VariantDefaultInitializer<double>::init(&value); |
There was a problem hiding this comment.
I think the shader language always uses 32-bit floats, although, in this case I guess we're initializing a Variant which will use a double internally? So, I guess this is right
There was a problem hiding this comment.
Yep, it's because there's no actual float variant.
Though i suppose it would still be possible to construct a float in a Variant, at least in the same way you'd assign it to a Variant.
Includes / supersedes #105246.
I recommend looking briefly at the above PR, but merging this one (without merging the above first), because it ultimately creates the better end picture.
Explanation
VariantGetInternalPtr,VariantImplicitConvert(introduced temporarily in #105246) andVariantInternalAccessordo pretty much the same thing: They allow users to get values and assign values when working withVariant, when the underlying type is known to match.Having 3 types for the same task is not a great idea, because code will diverge. In this PR, I am deduplicating functionality by deleting
VariantGetInternalPtrandVariantImplicitConvert, and merely usingVariantInternalAccessorin all cases.Doing this allows us to confidently maintain
VariantInternalAccessorgoing forwards, instead of having to maintain a convoluted mess of different ways to do the same thing.The PR is big and scary, but almost all of the changes are pretty safe search-and-replace.
Notes
Through search-and-replace, some
VariantInternalAccessoruses ended up using the following style:VariantInternalAccessor<A>::get(variant) = b; // Instead of VariantInternalAccessor<A>::set(variant, b);This looks a bit weird, but it's not detrimental to performance, using a reference to assign to the value. This can get cleaned up if it's confusing in the future.