godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.15k stars 97 forks source link

Expose Variant comparisons to C# #5758

Open dpalais opened 1 year ago

dpalais commented 1 year ago

Describe the project you are working on

I will implement a Goal Oriented Action Planning system for my game in Godot.

My intent is to be able to define and assign character actions, their precondition and effects checks inside the Godot editor. For that purpose I expect to be able to compare values, like the character's current state with the action's preconditions and effects, in order to select or discard the action for a potential future plan. The values to compare type might be an int, float, string, etc.

Describe the problem or limitation you are having in your project

Suppose you have this resource as an Action's precondition check definition.

Resource PreconditionCheck
        ComparisonOperator Operator;
        StringName Key
        Variant Value

In C# you should be able to compare the character's current state value with the action's precondition value regardless of the type of the Variant value. However, Variant to Variant comparisons aren't exposed in the C# Variant class so the following aren't possible:

bool check;
check = characterValue == actionPreconditionValue;
check = characterValue <= actionPreconditionValue;
check = characterValue > actionPreconditionValue;

C++ Variant also offers

static void evaluate(const Operator &p_op, const Variant &p_a, const Variant &p_b, Variant &r_ret, bool &r_valid);

However, none of these are available for use in C#.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Expose methods in C# to compare Variant values without casting them and without being aware of their type.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Doesn't apply.

If this enhancement will not be used often, can it be worked around with a few lines of script?

This can be worked around by explicitly knowing the type of the values to compare and using that knowledge in C# to cast the values to the appropriate type and then do the comparison in a case by case basis.

Is there a reason why this should be core and not an add-on in the asset library?

This is a C++ Variant feature that can only be made available in C# via the Godot mono module.

raulsntos commented 1 year ago

You should be able to implement this with extension methods, for example:

public static class VariantExtensions
{
    public static bool IsEqualsTo(this Variant left, Variant right)
    {
        if (left.VariantType != right.VariantType)
        {
            return false;
        }

        return switch left.VariantType
        {
            Variant.Type.Nil => true,
            Variant.Type.Bool => left.AsBool() == right.AsBool(),
            Variant.Type.Int => left.AsInt64() == right.AsInt64(),
            Variant.Type.Float => left.AsDouble() == right.AsDouble(),
            // ...
        }
    }
}

Basically you can use the Variant.VariantType property to determine the type of a Variant and then get the value as the correct type and compare them since at that point they are normal C# types.

You can also choose to ignore Variant.VariantType and just convert to the type you want anyway which is valid because Variant allows that, for example your variant can be a double like 4.2 and if you use .AsInt64() that will give you 4 which is a lossy conversion, at that point you basically lose type safety.

Being strict about the type results in a comparison like the === operator in JavaScript and ignoring the type results in a comparison like the == operator in JavaScript. I don't know what your requirements are, do you need 2 and "2" to be considered equal? What about 2 and 2.0?


Alternatively, if you are implementing your system in C# you might prefer to use generics and the IEquatable<T> interface, for example:

public bool CompareTwoValues<T>(T left, T right) where T : IEquatable<T>
{
    return left.Equals(right);
}

If you need, you can restrict T to only allow types that are compatible with Variant by using the [MustBeVariant] attribute:

public bool CompareTwoValues<[MustBeVariant] T>(T left, T right) where T : IEquatable
{
    return left.Equals(right);
}

All primitive C# types and Godot structs implement IEquatable<T>.

You may also consider implementing a IEqualityComparer<Variant> instead.


Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Doesn't apply.

It does apply. Proposals are meant to propose a solution to an existing problem; otherwise, it might be a better fit for a discussion. Are you asking to implement IEquatable<Variant> in Variant? Or the Variant::evaluate method from C++? Or the ==, >, <, <=, and >= operators?

Is there a reason why this should be core and not an add-on in the asset library?

This is a C++ Variant feature that can only be made available in C# via the Godot mono module.

As described above, this can be worked around in multiple ways depending on the requirements of your system. I personally don't consider this necessary to be in core and wouldn't want to encourage more usage of untyped APIs than what's already available in Godot APIs.

Also, I wouldn't use the Variant type for anything other than interop with Godot APIs. The Variant type was implemented because a lot of Godot APIs are untyped so we needed some type to use there in C# and System.Object (which is what we use in 3.x instead) was too open (which resulted in runtime errors when you use a type that's not compatible with Variant and bad performance from boxing value types).

MikeSchulze commented 1 year ago

Do you really force us to write your own Equals implementation? For simple types this is not the problem, but when Variant is an object it needs much more effort. Basic problem is still that GodotObjects don't implement Equals either and here you are again forced to write a Deep compare over all members.

AThousandShips commented 1 year ago

@MikeSchulze please, this isn't a constructive attitude

MikeSchulze commented 1 year ago

please, this isn't a constructive attitude

Yeah, sorry, I'm a little frustrated. The changes to Variant are making it very difficult for me to make adjustments to my unit test framework. It is essential for unit tests that you can compare objects and not only on reference equality.

Therefore I am forced to implement my own Equals methods that can handle Variant. But this turns out to be very extensive, even with ObjectTypes the game starts all over again, because there are no standard Equals implementations. This goes so far that I am forced to get all fields/paramaters via reflections and have to compare them manually,

I also don't understand why they implemented Godot Variant for the C#, I thought it was just trained to the GdScript Type. If all Godot objecte derive from GodotObject is but the type safety given or am I missing something?

Update. I'm unboxing the variants manually now, you can see what additional treatment this needs (still in progress) With Equals and ReferenceEquals provided for all Godot types as default, life could be so easy :) If you see a better way, am grateful for any help.

https://github.com/MikeSchulze/gdUnit4Mono/pull/7/files#diff-43215daea5aba967bd9429bde3028417c854528e7567007ca507ed68a63d3fcb

waldnercharles commented 1 year ago

As an exercise in patience, I wrote an extension method with every type in Godot 4's VariantType enum. I'll post it here in case it helps anyone else. This may also be a good candidate for a SourceGenerator in case any new types get added in the future.

public static bool VariantEquals(this Variant left, Variant right)
{
    if (left.VariantType != right.VariantType)
    {
        return false;
    }

    return left.VariantType switch
    {
        Variant.Type.Nil => true,
        Variant.Type.Bool => left.AsBool().Equals(right.AsBool()),
        Variant.Type.Int => left.AsInt64().Equals(right.AsInt64()),
        Variant.Type.Float => left.AsDouble().Equals(right.AsDouble()),
        Variant.Type.String => left.AsString().Equals(right.AsString()),
        Variant.Type.Vector2 => left.AsVector2().Equals(right.AsVector2()),
        Variant.Type.Vector2I => left.AsVector2I().Equals(right.AsVector2I()),
        Variant.Type.Rect2 => left.AsRect2().Equals(right.AsRect2()),
        Variant.Type.Rect2I => left.AsRect2I().Equals(right.AsRect2I()),
        Variant.Type.Vector3 => left.AsVector3().Equals(right.AsVector3()),
        Variant.Type.Vector3I => left.AsVector3I().Equals(right.AsVector3I()),
        Variant.Type.Transform2D => left.AsTransform2D().Equals(right.AsTransform2D()),
        Variant.Type.Vector4 => left.AsVector4().Equals(right.AsVector4()),
        Variant.Type.Vector4I => left.AsVector4I().Equals(right.AsVector4I()),
        Variant.Type.Plane => left.AsPlane().Equals(right.AsPlane()),
        Variant.Type.Quaternion => left.AsQuaternion().Equals(right.AsQuaternion()),
        Variant.Type.Aabb => left.AsAabb().Equals(right.AsAabb()),
        Variant.Type.Basis => left.AsBasis().Equals(right.AsBasis()),
        Variant.Type.Transform3D => left.AsTransform3D().Equals(right.AsTransform3D()),
        Variant.Type.Projection => left.AsProjection().Equals(right.AsProjection()),
        Variant.Type.Color => left.AsColor().Equals(right.AsColor()),
        Variant.Type.StringName => left.AsStringName().Equals(right.AsStringName()),
        Variant.Type.NodePath => left.AsNodePath().Equals(right.AsNodePath()),
        Variant.Type.Rid => left.AsRid().Equals(right.AsRid()),
        Variant.Type.Object => left.AsGodotObject().Equals(right.AsGodotObject()),
        Variant.Type.Callable => left.AsCallable().Equals(right),
        Variant.Type.Signal => left.AsSignal().Equals(right.AsSignal()),
        Variant.Type.Dictionary => left.AsGodotDictionary().Equals(right.AsGodotDictionary()),
        Variant.Type.Array => left.AsGodotArray().Equals(right.AsGodotArray()),
        Variant.Type.PackedByteArray => left.AsByteArray().Equals(right.AsByteArray()),
        Variant.Type.PackedInt32Array => left.AsInt32Array().Equals(right.AsInt32Array()),
        Variant.Type.PackedInt64Array => left.AsInt64Array().Equals(right.AsInt64Array()),
        Variant.Type.PackedFloat32Array => left.AsFloat32Array().Equals(right.AsFloat32Array()),
        Variant.Type.PackedFloat64Array => left.AsFloat64Array().Equals(right.AsFloat64Array()),
        Variant.Type.PackedStringArray => left.AsStringArray().Equals(right.AsStringArray()),
        Variant.Type.PackedVector2Array => left.AsVector2Array().Equals(right.AsVector2Array()),
        Variant.Type.PackedVector3Array => left.AsVector3Array().Equals(right.AsVector3Array()),
        Variant.Type.PackedColorArray => left.AsColorArray().Equals(right.AsColorArray()),
        _ => throw new ArgumentOutOfRangeException(nameof(left))
    };
}
MikeSchulze commented 1 year ago

@waldnercharles thanks for your anwser but this code will not realy help. All object types are do not implement equals and is working like a ReverenceEquals and is in my eyes wrong. At least the Variant should implement the Equals to avoid such custom code.

waldnercharles commented 1 year ago

@MikeSchulze That's fair. Pretty much all of the types are value types or handle their own Equals, so it will work in the majority of cases, but for the following types users may run into some issues:

Object, Callable (uses ValueType.Equals), Signal (usese ValueType.Equals), Dictionary, Array, and all of the PackedArrays.

In theory those could be handled using a SequenceEquals or something if someone wants, but, I just figured I'd write the code out that was mentioned by @raulsntos to save others that see this issue some time.

I agree that Variant should implement its own Equals, but, I actually think it should conform to the same way that Equals behaves on other reference types when the underlying value is a reference type.

MikeSchulze commented 7 months ago

Is there any update ? I found also an issue when using the unboxed values.

        Godot.Variant v1 = "a1";
        Godot.Variant v2 = "a1";

        Console.WriteLine(Equals(v1.AsString(), v2.AsString()));
        Console.WriteLine(ReferenceEquals(v1.AsString(), v2.AsString()));
        Console.WriteLine(Equals("a1", "a1"));
        Console.WriteLine(ReferenceEquals("a1", "a1"));

it results in

 stdout: True
stdout: False
stdout: True
stdout: True

but it should be true in all cases

AThousandShips commented 7 months ago

Why should ReferenceEquals be true? Is it not to check that they hold the same reference?

Determines whether the specified Object instances are the same instance

But they don't, because they're two different instances, the last line compares two constants, that's different

What happens with:

Godot.Variant v1 = "A1";
Godot.Variant v2 = v1;

For it to return true in the first case it'd have to find the same string object, so it'd have to look that up somewhere, that wouldn't be very good for performance

MikeSchulze commented 7 months ago

Why should ReferenceEquals be true? Is it not to check that they hold the same reference?

Of course, since v1 and v2 are of type Variant and only represent an artificial shell for the string objects, they should also be identical by reference. The strings are "constant strings", i.e. identical by reference. It should be work equal to ReferenceEquals("a1", "a1")

But presumably the values held by Variant are not considered constants, which means that there is no reference equal.

AThousandShips commented 7 months ago

Why would they be considered constant? They're assigned to non-constant values? That's a huge assumption by the compiler to make, they aren't constants, they're variables assigned from constants, that's very different, and they hold different data underneath so they're converted to a new string

They're not wrappers around string, they're wrappers around godot_string, which isn't the same type

The main reason here is that string and godot_string are not the same encoding, godot_string, like the String in the engine, is UTF-32, but string in C# is UTF-16

MikeSchulze commented 7 months ago

Ah, now I understand, thanks for the explanation. I must say that I have a Java background and strings are always reference equations. Hence, my misunderstanding, but thanks for your time. 👍

AThousandShips commented 7 months ago

In this case I think it's relevant to consider that Variant is designed for interopt with the engine, so it's designed to use engine side compatible data, so setting data on it will convert it if it's not directly compatible, it's best to use C# native types when you're not interested in interopt directly

MikeSchulze commented 7 months ago

Unfortunately this is not so easy, I am currently actively working on the GdUnit4 C# API, and it needs to make sure that the asserts handle both C# native and Godot types. Hence, the request for equals on Variant which would make my life easier. Currently, I have implemented boxing and unboxing for all types which works quite well. I run actual into this issue because I am extending the asserts with kind of IsSame verification methods.