Skip to content

Derive Hash for integer Vector types #356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

B4rc1
Copy link
Contributor

@B4rc1 B4rc1 commented Jul 23, 2023

Related: #297

My Usecase: Putting Vector2i's in a HashMap

@lilizoey
Copy link
Member

Hm, do you think it would be better for us to reuse godots hash implementation here?

@lilizoey lilizoey added feature Adds functionality to the library c: core Core components labels Jul 23, 2023
@B4rc1
Copy link
Contributor Author

B4rc1 commented Jul 23, 2023

I think for more complex data types such as Vector2 or a general (macro assisted) solution, yes, because e.g. f32 does not implement hash by default.

But for this specific case deriving should be fine as hashes are never shared across the FFI-Interface (correct me if I'm wrong) and there are no conventions to consider.

A convention would be the case of a NaN float, should all NaN floats be the same? Should they all be different? Should they be different if the mantissa/exponent is different?

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-356

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a useful addition, thanks! One minor comment (please amend the 1 commit instead of adding a new one).

As for the question whether we should use Godot's hash, it's probably a wider discussion. Things to consider:

  • Eq + Hash + Ord must play together in a consistent way.
  • If we use Godot's impl, we need to either call FFI or maintain a separate implementation, both of which come with drawbacks.
  • Godot's impl may have different trade-offs regarding performance or security (not sure how applicable in gamedev domain, but something to keep in mind).
  • At the moment, Variant::from(x) and x have different hashing and equality relations, which may or may not be OK.

These are for a separate discussion though; a feature request with use case + rationale would be nice. Either way, making integer vectors hashable is relatively uncontroversial. The implementation can also be changed later, which should be transparent to the user. Or we could consider adding special functions or wrapper types for Godot-based equality/ordering/hashing.

@@ -21,7 +21,7 @@ use std::fmt;
/// required. Note that the values are limited to 32 bits, and unlike [`Vector2`] this cannot be
/// configured with an engine build option. Use `i64` or [`PackedInt64Array`] if 64-bit values are
/// needed.
#[derive(Default, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Debug)]
#[derive(Default, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Debug, Hash)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Hash should come before Debug.

(I still need to write these conventions down somewhere...)

@B4rc1 B4rc1 force-pushed the derive-hash-integer-vectors branch from c6339ea to a2e6f0c Compare July 24, 2023 09:32
@Bromeon
Copy link
Member

Bromeon commented Jul 24, 2023

Thanks!

@Bromeon Bromeon enabled auto-merge July 24, 2023 15:54
@Bromeon Bromeon added this pull request to the merge queue Jul 24, 2023
Merged via the queue into godot-rust:master with commit a6c5184 Jul 24, 2023
@B4rc1 B4rc1 deleted the derive-hash-integer-vectors branch July 25, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants