Inconsistent Godot and C# (system) Dictionary class

[mnn]

Godot version:
3.1.beta3.mono.official

OS/device including version:
Linux 64b, Kubuntu 16.04

Issue description:
System.Collections.Generic.Dictionary and Godot.Collections.Dictionary are not behaving same when it comes to key comparison.

using Godot;
using System;
using SC = System.Collections.Generic;
using GC = Godot.Collections;
using Object = Godot.Object;

public class Key: Object {
  public string name;
  public Key(string name) { this.name = name; }

  // generated
  protected bool Equals(Key other) {
    return string.Equals(name, other.name);
  }

  // generated
  public override bool Equals(object obj) {
    if(ReferenceEquals(null, obj)) return false;
    if(ReferenceEquals(this, obj)) return true;
    if(obj.GetType() != this.GetType()) return false;
    return Equals((Key) obj);
  }

  // generated
  public override int GetHashCode() {
    return (name != null ? name.GetHashCode() : 0);
  }
}

public class Dicts: Node {
  private static void Assert(bool cond) {
    if(!cond) { throw new Exception("Assert failed."); }
  }

  public override void _Ready() {
    GD.Print("Dicts.cs");

    var origKey = new Key("A");
    var otherKeyWithSameValue = new Key("A");
    Assert(origKey != otherKeyWithSameValue);
    Assert(origKey.Equals(otherKeyWithSameValue));
    var sysDict = new SC.Dictionary<Key, int>();
    var godotDict = new GC.Dictionary<Key, int>();

    Assert(!sysDict.ContainsKey(origKey));
    Assert(!godotDict.ContainsKey(origKey));
    sysDict.Add(origKey, 1);
    godotDict.Add(origKey, 1);
    Assert(sysDict.ContainsKey(origKey));
    Assert(godotDict.ContainsKey(origKey));
    Assert(sysDict.ContainsKey(otherKeyWithSameValue));
    Assert(godotDict.ContainsKey(otherKeyWithSameValue)); // doesn't hold, crashes
  }
}

Is it possible to have a custom class as a key in a Godot Dictionary? At first I wanted to use struct, but Godot Dictionary kept refusing it, so I changed it to a class and it seemed to be working. Until I created a new key with values of one existing key in the dictionary and tried to get data from the dictionary. Is this a limitation, bug or intention? Is it even possible to fix (I really have no idea how data are passed between engine and C# part)?

By the way, the custom key is Vector3Int. I still can't comprehend why Godot is missing such a basic thing (for years?).

Steps to reproduce:
Attach the above script to a node in a project and run.

[neikeq]

Dictionary and Array expect objects that can be marshalled as Variant (which includes classes that inherit from Godot.Object). The idea is to allow other objects as well in the future, but it's not possible yet.

From what I understand, you expect ContainsKey to call Equals when comparing two objects. The problem is these classes are wrapper for native collections that store Variant elements. Godot.Dictionary.ContainsKey calls the C++ method Dictionary.has, which checks for Variant equality. It has no notion of the managed objects.

My recommendation is, only use the collections in Godot.Collections if you really need them. The cases I can think of are passing a mutable collection to the engine (as it's done with OS.Execute to receive the output) or in cases where you pass the collection to the engine too regularly that doing a copy every time is too much overhead.

Right now, only Godot.Collections and raw arrays are supported for marshalling so trying to, for example, export a field of other collection type will not work, but this will be addressed soon.

[mnn]

From what I understand, you expect ContainsKey to call Equals when comparing two objects.

Yes, exactly. Now Equals (and I am guessing GetHashCode too) is not used at all - breaking contract in C# (Mono) for how comparison generally works.

The problem is these classes are wrapper for native collections that store Variant elements. Godot.Dictionary.ContainsKey calls the C++ method Dictionary.has, which checks for Variant equality. It has no notion of the managed objects.

I don't think it has to have knowledge of an object being a managed object. Shouldn’t be enough if engine supported on general object (Variant? I mean the class type from GDScript which managed objects take form of) Equals (or similar) method which could be overridden? If I understand it correctly, GDScript doesn't have anything like that, so the problem is in GDScript itself too? Or does it use a deep structural comparison of dictionaries and custom classes for testing equality (so also when used as a key in a dictionary)?

My recommendation is, only use the collections in Godot.Collections if you really need them. The cases I can think of are passing a mutable collection to the engine (as it's done with OS.Execute to receive the output) or in cases where you pass the collection to the engine too regularly that doing a copy every time is too much overhead.

Well, I wanted to have only computationally demanding tasks in C# (or C++) and sharing results to GDScript where I could do everything else. But a dictionary which cannot properly test equality is a problem (I really don't want to manage a pool of all possible keys, which will heavily change over time, since I believe it would be quite bad for performance [it will be used in many tight loops]). I think converting between dictionaries (Mono and Godot one) will be quite costly. And another issue is struct is not supported as a key as well. It seems to me that I will have to do majority of code in C# (or C++), hold all state there, and in GDScript do only small bits of glue 😞.

[neikeq]

Variant's OP_EQUALS case for TYPE_OBJECT could be made to fallback to scripts for checking equality. I don't know if such change would be accepted and I also don't know if the performance penalty would be worth it.

[neikeq]

Would it be enough for ContainsKey to support Equals or would it also be a problem if has in GDScript or the engine didn't support it?
If ContainsKey is enough, then we could have a special case for this.

[mnn]

The ContainsKey was used just as an example, at least also removing item (by key) and getting value by key from the dictionary would be needed (that's how I found out about this problem - key class supported equality, I saw the key was in a dictionary, yet it crashed when trying to get it using new equal key). My one use case is that in GDScript I would construct the key and get an object from the dictionary, get a field (3 dimensional array) from that object and from that get an integer (more work with this value would follow). Not sure at all if there wouldn't be more issues, or if I wouldn't hit performance limits of GDScript. (My very focused bench showed it being slower by ~11 times compared to C# [which might be fast enough for me], but I saw several other people stating much much higher numbers.)

If I am the only one asking about this, I am not sure adding a special case (bringing inconsistency) is worth it. The option of overriding equality checking for objects would be great, but I don't know anything about engine's internals, so if you say the cost is probably too high compared to what it brings, I wouldn't pursue this path.

Thank you for your time 🙂.