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_internal.hpp. #1655

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Nov 26, 2024

This module contains VariantInternalType, VariantInternal, VariantGetInternalPtr, VariantInternalAccessor and VariantDefaultInitializer, facilitating access and manipulation of a Variant's internal value in various ways.

The reference godot variant_internal.hpp is here. I chose a different, less boilerplate-y approach to implementation. It should work the same though. Doing it this way should decrease maintenance (and implementation) effort, since most of these are just copy-pasted code without any type specific logic (with the exception of Object *).

There are a few small differences:

  • VariantInternalType does not exist in godot's implementation. I added it because it makes implementation of the other functions easier.
  • get_object returns Object * const * in my implementation, but const Object ** in godot's. Technically speaking, godot's implementation is wrong, because it allows manipulation of the contained Object *, even though a const Variant was supplied. Godot uses a C-style cast that acts as a const_cast to avoid cast issues.
  • VariantInternalAccessor is not implemented for Object *, even though it is in Godot. I believe it is not possible to replicate the intended behavior without access to Variant::object_assign (or internal ObjData). I made it fail statically using SFINAE.

I think the rest should be the same.

PR contains a unit test for one of the internal accessors.

Depends on #1654.

@Ivorforce Ivorforce requested a review from a team as a code owner November 26, 2024 22:49
@Ivorforce Ivorforce force-pushed the variant-internal branch 3 times, most recently from 32f4df3 to 332690f Compare November 28, 2024 13:34
@dsnopek
Copy link
Collaborator

dsnopek commented Nov 28, 2024

Thanks!

At a high-level, this looks great!

  • VariantInternalType does not exist in godot's implementation. I added it because it makes implementation of the other functions easier.

In order to discourage developers from using it (and create incompatibilities with Godot modules) you could put it in the godot::internal namespace? We do that in some other places, although, we should probably use it even more than we do.

  • get_object returns Object * const * in my implementation, but const Object ** in godot's. Technically speaking, godot's implementation is wrong, because it allows manipulation of the contained Object *, even though a const Variant was supplied. Godot uses a C-style cast that acts as a const_cast to avoid cast issues.

I don't know about this. Since auto is discouraged in Godot modules, this would potentially make code incompatible between godot-cpp and and a Godot module.

I'd personally prefer to match Godot (even if it's wrong), and separately make an issue or PR for Godot to fix it there, and then we can match it in godot-cpp after.

  • VariantInternalAccessor is not implemented for Object *, even though it is in Godot. I believe it is not possible to replicate the intended behavior without access to Variant::object_assign (or internal ObjData). I made it fail statically using SFINAE.

Hm, it sounds like the problem is only for setting with Object *? Is there a way we could still support getting?

include/godot_cpp/variant/variant_internal.hpp Outdated Show resolved Hide resolved
test/project/main.gd Show resolved Hide resolved
@Ivorforce Ivorforce force-pushed the variant-internal branch 2 times, most recently from 2a05ce5 to 6a96858 Compare November 28, 2024 14:05
@Ivorforce
Copy link
Contributor Author

Ivorforce commented Nov 28, 2024

In order to discourage developers from using it (and create incompatibilities with Godot modules) you could put it in the godot::internal namespace? We do that in some other places, although, we should probably use it even more than we do.

That's fine by me. Done!

  • get_object returns Object * const * in my implementation, but const Object ** in godot's. Technically speaking, godot's implementation is wrong, because it allows manipulation of the contained Object *, even though a const Variant was supplied. Godot uses a C-style cast that acts as a const_cast to avoid cast issues.

I don't know about this. Since auto is discouraged in Godot modules, this would potentially make code incompatible between godot-cpp and and a Godot module.
I'd personally prefer to match Godot (even if it's wrong), and separately make an issue or PR for Godot to fix it there, and then we can match it in godot-cpp after.

Ok. I changed it to a cast-return to match godot's interface.

  • VariantInternalAccessor is not implemented for Object *, even though it is in Godot. I believe it is not possible to replicate the intended behavior without access to Variant::object_assign (or internal ObjData). I made it fail statically using SFINAE.

Hm, it sounds like the problem is only for setting with Object *? Is there a way we could still support getting?

Yep. I adjusted the PR accordingly.

This module contains VariantInternalType, VariantInternal, VariantGetInternalPtr, VariantInternalAccessor and VariantDefaultInitializer, allowing to access and manipulate Variant's internal values.
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! The latest version of this looks good to me :-)

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Dec 2, 2024
@dsnopek dsnopek added this to the 4.x milestone Dec 2, 2024
@dsnopek dsnopek merged commit 72aeb35 into godotengine:master Dec 6, 2024
11 checks passed
@Ivorforce Ivorforce deleted the variant-internal branch December 6, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants