godot-rust / gdnative

Rust bindings for Godot 3
https://godot-rust.github.io
MIT License
3.62k stars 210 forks source link

Impl `PartialEq` for `Dictionary` and `VariantArray` #990

Open chitoyuu opened 1 year ago

chitoyuu commented 1 year ago

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.

Bromeon commented 1 year ago

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

chitoyuu commented 1 year ago

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.

Bromeon commented 1 year ago

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:

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.