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.

Sign up for GitHub

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

Jump to bottom

[Core] Add is_same to types that have float components #97553

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Core] Add is_same to types that have float components #97553

wants to merge 1 commit into from
+103 −91

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Sep 27, 2024 •
edited
Loading

Compares NaN as equal.

Added to:

  • AABB
  • Basis
  • Color
  • Quaternion
  • Plane
  • Projection
  • Rect2
  • Transform2D/3D
  • Vector2
  • Vector3
  • Vector4

And a helper method in Math

Based on the code in Variant::hash_compare

Replaces the code in HashMapComparatorDefault and Variant::hash_compare

Can expose these as well as I think they are useful for script

Depends on (see for context):

@AThousandShips AThousandShips added this to the 4.x milestone Sep 27, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner September 27, 2024 17:40
@AThousandShips AThousandShips mentioned this pull request Sep 27, 2024
Merged
@AThousandShips
Copy link
Member Author

The name is entirely up for debate, picked it based on the naming in hash_compare but makes sense IMO

@AThousandShips
Copy link
Member Author

Will move the (a == b) || (Math::is_nan(a) && Math::is_nan(b)) to Math to avoid the duplication here

@AThousandShips AThousandShips force-pushed the semantic_equal branch 2 times, most recently from 21ec318 to b294b14 Compare September 27, 2024 18:44
@fire
Copy link
Member

fire commented Sep 28, 2024

Why is nan comparison equal? Curious.

@AThousandShips
Copy link
Member Author

AThousandShips commented Sep 28, 2024 •
edited
Loading

For the special uses, otherwise you can't remove Vector2(NAN, NAN) from a HashMap for example, hence the hash_compare and the default comparisons

We already technically have this for float in is_same, so could use that naming instead, it uses Variant::identity_compare which calles hash_compare for these types

Repiteo
Repiteo reviewed Sep 28, 2024
View reviewed changes
core/math/vector2.h Outdated
Comment on lines 131 to 133
bool is_equal_approx(const Vector2 &p_v) const;
bool is_semantic_equal(const Vector2 &p_v) const;
bool is_zero_approx() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the syntax of the surrounding functions (both here and elsewhere), maybe is_equal_semantic would be more appropriate?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm considering using is_same as I'd say it's more clear, only drawback is that if we expose these the float one in scripting won't be available, and you'd have to use is_same which is a dynamic method

Copy link
Contributor

Choose a reason for hiding this comment

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

What float method are you referring to? The only is_same I'm aware of is the static method, which wouldn't overlap with this implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Meaning the scripting method

Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine that GDScript would have a means of binding a function to float; it'd be a bit unorthodox & probably more involved, but nothing outright inhibiting.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't bing methods called on float currently, but I think is_same works using the generic version

core/variant/variant.cpp Outdated Show resolved Hide resolved
@Repiteo
Copy link
Contributor

Repiteo commented Sep 28, 2024

Similar to #97550, it's probably worth adding these to the other Variant math structs for consistency. There's a fair bit of inconsistency in implementation across those scripts (structurally & functionally) that should be ironed out at some point, but that's not an immediate concern.

@AThousandShips
Copy link
Member Author

AThousandShips commented Sep 28, 2024 •
edited
Loading

Will add the remaining cases and rebase on top of an updated #97550

Will leave any exposing of these to the future and will change to is_same unless there's any opposition to that, more clear IMO

@AThousandShips AThousandShips changed the title [Core] Add is_semantic_equal to types that have float components [Core] Add is_same to types that have float components Sep 28, 2024
AThousandShips
AThousandShips commented Sep 28, 2024
View reviewed changes
core/variant/variant.cpp
Comment on lines +3266 to +3152
#define hash_compare_quaternion(p_lhs, p_rhs) \
(p_lhs).is_same(p_rhs)
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is now completely redundant, can remove but left it for now

@clayjohn
Copy link
Member

Can you briefly explain what benefit this PR provides? Is there a bug this fixes or something that this improves?

@AThousandShips
Copy link
Member Author

This is to unify uses of nan-handling equality checks used in hash comparisons, code that's duplicated between Variant and HashMapComparatorDefault, and possibly exposing these in the future (helping with the confusing cases with the == and != operators) to avoid hard to handle cases there

The primary reason is the noisy code added in #97550 to cover all relevant cases there

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Oct 3, 2024
@Repiteo Repiteo modified the milestones: 4.4, 4.x Jan 20, 2025
@AThousandShips AThousandShips modified the milestones: 4.x, 4.5 Jan 28, 2025
@AThousandShips
[Core] Add is_same to types that have float components
e825085 
Compares `NaN` as equal.

Added to:
* `AABB`
* `Basis`
* `Color`
* `Plane`
* `Projection`
* `Quaternion`
* `Rect2`
* `Transform2D`
* `Transform3D`
* `Vector2`
* `Vector3`
* `Vector4`

And added as a method in `Math`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Repiteo Repiteo Repiteo approved these changes

Assignees
No one assigned
Labels
enhancement feature proposal topic:core
Projects
None yet
Milestone
4.5
Development

Successfully merging this pull request may close these issues.
5 participants
@AThousandShips @fire @Repiteo @clayjohn @akien-mga