gdnative
gdnative copied to clipboard
Impl `PartialEq` for `Dictionary` and `VariantArray`
Created during survey of commented code (#377).
Dictionary
and VariantArray
are missing PartialEq
implementations. This is immediately required by the serde serialization test, but should be generally useful to end users as well.
One interesting decision here -- do we use Rust equality or Godot equality?
In most cases it should amount to the same, but I could imagine some subleties, especially with types that involve float
. I think it's good that you explicitly mention PartialEq
(and not Eq
).
That's good point you bring up. I'd lean toward using Godot's equality operator here to be in line with Variant
, but there is an important distinction: the operator==
for Dictionary
implements reference equality instead of value equality. Using Godot equality here would be consistent from an implementation viewpoint, but inconsistent from a behavior one.
This could be a good argument for an explicit wrapper as suggested here: https://github.com/godot-rust/gdnative/issues/712#issuecomment-858113913.
The original suggestion is about a separate type from Ref
, but with GATs stablized it should be much easier to write a single reference type encompassing everything now, so that's less of a concern.
the
operator==
forDictionary
implements reference equality instead of value equality. Using Godot equality here would be consistent from an implementation viewpoint, but inconsistent from a behavior one.
Noteworthy:
- This has been fixed in Godot 4:
Dictionary
now uses value equality (see PR). - At the same time, Godot 3 now has the
deep_equal
utility function (see PR).
I'm not sure if an explicit wrapper type is truly needed, or if a deep_equal
method would do the job as well.
I'd lean toward using Godot's equality operator here to be in line with
Variant
,
I'm not sure if the equivalence (a == b) == (a.to_variant() == b.to_variant())
currently holds for all cases.
I quickly tested, it holds for f32::NAN
and f64::NAN
though.
It eventually amounts to a choice between:
- Do we want to keep the broken GDScript behavior, but stay consistent with Godot (and Rust's
Variant
in its current implementation).- Helpful for people porting GDScript code or largely familiar with GDScript.
- Or do we want to make the default operation the intuitive one in Rust (value equality), while tripping up people porting GDScript code?
- Helpful for people starting with the Rust binding, or deliberately choosing Rust for better type safety.
- Should we even try to "fix"
Variant
equality one day?
I don't think godot-rust has very clearly favored one way over the other.