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

Add Variant value pointer getter functions, similar to get_validated_object. #98392

Closed

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Oct 21, 2024

Adds the ability to interact with Variant's underlying types in a strongly typed fashion.

Related to #98373: They two fill similar, but still somewhat orthogonal, use-cases.

Related to godotengine/godot-proposals#10830. This PR does not close the proposal, but partially addresses its use-cases. For more information, see my discussion with @dsnopek in the proposal.

@Ivorforce Ivorforce requested a review from a team as a code owner October 21, 2024 19:20
@Ivorforce Ivorforce force-pushed the packed-array-lvalue-ref branch 4 times, most recently from 0a1e7bf to b4c0a6b Compare October 21, 2024 20:40
@Ivorforce
Copy link
Member Author

I needed to say goodbye to lvalue references. Besides the rather explicit crashes on conversions, the explicit lvalue conversions were being used automatically leading to unit test crashes already.

Pointers make somewhat more sense anyway because it can be nullable, which is fitting in this case, requiring no explicit crash.

@Ivorforce Ivorforce force-pushed the packed-array-lvalue-ref branch from 20254cd to 8f011e4 Compare October 22, 2024 10:58
@Ivorforce Ivorforce changed the title Add explicit conversions from Variant to Packed array references. Add Variant value pointer getter functions. Oct 22, 2024
@Ivorforce
Copy link
Member Author

Ivorforce commented Oct 22, 2024

Ok, I refactored to use variant.get_xxx notation instead. This is closer to std::get<XXX>(variant) syntax, with the main difference being that we return a pointer rather than an lvalue reference, which is null on type failure rather than crashing.

I also added getters for all the other types, except Object which already has such a getter (get_validated_object()). This will allow in-place modification and non-copy use of all held types. Keep in mind this is not new behavior: Variant can already be modified in-place with call. The main addition in this PR is to allow the modification to be typesafe.

@Ivorforce Ivorforce changed the title Add Variant value pointer getter functions. Add Variant value pointer getter functions, similar to get_validated_object. Oct 22, 2024
@dsnopek
Copy link
Contributor

dsnopek commented Oct 29, 2024

These functions seem basically the same as the VariantInternal::get_*() functions.

Given that we want to expose something like this on the GDExtension side, I'm not sure what the API should be. We could certainly make matching VariantInternal::get_*() functions in godot-cpp, so long as Godot provided us with a way to get the underlying pointers, although, perhaps that isn't the friendliest API?

So, probably the first thing is we need to decide on what API we want within Godot, and then we can figure out how to make the same work in GDExtension and godot-cpp.

@Ivorforce
Copy link
Member Author

Ivorforce commented Nov 12, 2024

These functions seem basically the same as the VariantInternal::get_*() functions.

Oh, I had no idea those existed! There remains a small difference in that my implementation checks for variant state first. This may or may not be wanted; the std::get<>() API crashes when it's in a wrong state, and most uses of this API would probably also not check for null before de-referencing. I'm not sure what I prefer.

Given that we want to expose something like this on the GDExtension side, I'm not sure what the API should be. We could certainly make matching VariantInternal::get_*() functions in godot-cpp, so long as Godot provided us with a way to get the underlying pointers, although, perhaps that isn't the friendliest API?

This would be blocked by Variant not exposing PackedArrayRef, a change proposed in #98373 but not accepted (or sufficiently discussed) yet.

So, probably the first thing is we need to decide on what API we want within Godot, and then we can figure out how to make the same work in GDExtension and godot-cpp.

There seem to be a few scattered uses of the *VariantInternal::get_xxx(variant) API in some spots, but mostly *VariantGetInternalPtr<xxx>::get_ptr(variant) seems to be used (which uses the above internally). That's probably because a lot of the code using it is using templates, and using this code to abstract its logic.

@Ivorforce
Copy link
Member Author

Superseded by #99201 because Variant internal has the proposed functionality already, an equivalent just needs to be exposed to gdextension.

@Ivorforce Ivorforce closed this Nov 17, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone Nov 17, 2024
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.

3 participants