gdnative icon indicating copy to clipboard operation
gdnative copied to clipboard

Impl `PartialEq` for `Dictionary` and `VariantArray`

Open chitoyuu opened this issue 2 years ago • 3 comments

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.

chitoyuu avatar Dec 07 '22 07:12 chitoyuu

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).

Bromeon avatar Dec 07 '22 11:12 Bromeon

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.

chitoyuu avatar Dec 07 '22 12:12 chitoyuu

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.

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:

  1. 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.
  2. 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.

Bromeon avatar Dec 07 '22 18:12 Bromeon