godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.79k stars 20.91k forks source link

Geometry functions returning null Variants in GDScript returns (possibly) false values in C# #89850

Open PerMalmberg opened 6 months ago

PerMalmberg commented 6 months ago

Tested versions

System information

Godot v4.2.1.stable.mono - Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2070 SUPER (NVIDIA; 31.0.15.5176) - AMD Ryzen 7 3800XT 8-Core Processor (16 Threads)

Issue description

There may be other instances where this happens, but these are the ones I've found:

Vector2/3.Zero may be the actual intersection point so there is no way can check if an intersection actually exists or not using these functions in C#.

Steps to reproduce


using static Godot.Geometry2D;
....
// Correctly returns (0,0)
var undeterminable = SegmentIntersectsSegment(new Vector2(-1,0), new Vector2(1, 0), new Vector2(0, -1), new Vector2(0, 1)).AsVector2();

// Erroneously returns (0,0)
undeterminable = SegmentIntersectsSegment(new Vector2(-1, 0), new Vector2(0, 0), new Vector2(1, 1), new Vector2(1, 2)).AsVector2();

// 
// CS0019   Operator '==' cannot be applied to operands of type 'Variant' and '<null>'
// if(undeterminable == null) { 
//
// }

The two first vectors intersect at (0,0), but we can't determine that since it might also be a default constructed Vector2, i.e. null. The second two also results in (0,0)

Minimal reproduction project (MRP)

See steps to reproduce.

Edit: Make example code more explicit for Vector2.

PerMalmberg commented 5 months ago

Here's a MRP showing the problem and the entire C# code for those that don't want to open the actual project.

https://github.com/godotengine/godot/assets/2729388/271db3e6-3ad9-4a38-84bc-4f24e0015fcc

Godot-89850.zip

using Godot;
using System.Linq;
using static Godot.Geometry2D;

public partial class Node2D : Godot.Node2D
{
    private Vector2[] crossing = [new Vector2(100, 100), new Vector2(200, 100),
                                  new Vector2(150, 50), new Vector2(150, 150)];

    private Vector2[] notCrossing = [new Vector2(100, 100), new Vector2(100, 200),
                                  new Vector2(150, 100), new Vector2(150, 200)];

    private Vector2[] current;

    // Called when the node enters the scene tree for the first time.
    public override void _Ready()
    {
        Toggle();
    }

    private void Toggle()
    {
        current = current == notCrossing ? crossing : notCrossing;
        var l1 = GetNode<Line2D>("%Line1");
        var l2 = GetNode<Line2D>("%Line2");

        var points1 = current.Take(2).ToArray();
        var points2 = current.TakeLast(2).ToArray();

        l1.Points = points1;
        l2.Points = points2;
        var intersection = SegmentIntersectsSegment(points1[0], points1[1], points2[0], points2[1]).As<Vector2>();
        GetNode<Label>("%Label").Text = intersection.ToString();
    }

    public void on_pressed()
    {
        Toggle();        
    }
}
raulsntos commented 5 months ago

Vector2/3.Zero may be the actual intersection point so there is no way can check if an intersection actually exists or not using these functions in C#.

If a Variant is null, you can check the type:

Variant intersection = SegmentIntersectSegment(fromA, toA, fromB, toB);
if (intersection.VariantType == Variant.Type.Nil)
{
    // Intersection is null.
}
else
{
    // Intersection is not null.
    Vector2 intersectionVector = intersection.As<Vector2>();
}
PerMalmberg commented 5 months ago

Ok, that's a step in the right direction.

For now we can wrap it like this:

private Vector2? SegmentIntersectsSegment2(Vector2 a1, Vector2 a2, Vector2 b1, Vector2 b2)
{
    var intersection = SegmentIntersectsSegment(a1, a2, b1, b2); ;
    return intersection.VariantType == Variant.Type.Nil ? null : intersection.AsVector2();        
}

May I suggest that these functions are changed to return a Vector2? instead of a Variant, essentially providing the above wrapper as part of the API? That would bring them in line with what the documentation states and be more what is expected, imho.

raulsntos commented 5 months ago

These methods are generated from the information in ClassDB and ClassDB only says that the return type is Variant. We have no way of knowing if that means Vector2 | null or Vector2 | Vector3 or any other combination of types.

This information may be added to ClassDB if this proposal gets implemented:

But keep in mind that changing the return type of a method breaks binary compatibility in C#.

PerMalmberg commented 5 months ago

Yes, it is a breaking change. And needs to be treated accordingly. It has been done previously though, see https://github.com/godotengine/godot/pull/32670