Skip to content
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

[Core] Add missing HashMapComparatorDefault cases #97550

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Sep 27, 2024

Added for:

  • AABB
  • Basis
  • Color
  • Plane
  • Projection
  • Quaternion
  • Rect2
  • Transform2D
  • Transform3D
  • Vector4

Ensures these handle NaN correctly

@AThousandShips
Copy link
Member Author

AThousandShips commented Sep 27, 2024

We should probably add these as methods on VectorX to avoid this being here, and we can merge the macros used in variant.cpp

Done:

@AThousandShips
Copy link
Member Author

AThousandShips commented Sep 27, 2024

Realized Quaternion and Color are missing as well (compared to Variant::hash_compare), will add

Could probably add for the following as well:

  • AABB
  • Basis
  • Projection
  • Rect2
  • Transform2D/3D

Matching the code in Variant::hash_compare

But unsure if these are necessary or used

@AThousandShips AThousandShips changed the title [Core] Add missing HashMapComparatorDefault<Vector4> [Core] Add missing HashMapComparatorDefault for Vector4, Quaternion, and Color Sep 27, 2024
@AThousandShips AThousandShips force-pushed the hash_fix branch 2 times, most recently from d0a45e4 to 29c2261 Compare September 27, 2024 17:34
@Repiteo
Copy link
Contributor

Repiteo commented Sep 28, 2024

I'd go ahead and add it for all of the Variant math structs; don't see any real reason not to do so.

@AThousandShips
Copy link
Member Author

Will do! It'll ensure complete stability

Added for:
* `AABB`
* `Basis`
* `Color`
* `Plane`
* `Projection`
* `Quaternion`
* `Rect2`
* `Transform2D`
* `Transform3D`
* `Vector4`

Ensures these handles `NaN` correctly
@AThousandShips AThousandShips changed the title [Core] Add missing HashMapComparatorDefault for Vector4, Quaternion, and Color [Core] Add missing HashMapComparatorDefault cases Sep 28, 2024
template <>
struct HashMapComparatorDefault<Rect2> {
static bool compare(const Rect2 &p_lhs, const Rect2 &p_rhs) {
return HashMapComparatorDefault<Vector2>().compare(p_lhs.position, p_rhs.position) && HashMapComparatorDefault<Vector2>().compare(p_lhs.size, p_rhs.size);
Copy link
Member Author

Choose a reason for hiding this comment

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

Delegated here to avoid very complex expressions (will hopefully be unnecessary with #97553)

Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

#97553 will make any extra verbosity irrelevant, so this works as-is.

@akien-mga akien-mga merged commit 86b1514 into godotengine:master Oct 2, 2024
19 checks passed
@AThousandShips AThousandShips deleted the hash_fix branch October 2, 2024 13:26
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants