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] Fix AABB.encloses failing on shared upper bound #87118

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jan 12, 2024

This differs from Rect2(i) and was fixed for those classes in the past

See:

Which fixed this for those types, makes sense to me to have parity between the two, and it doesn't make sense to only allow it on one side, or fail to enclose yourself. I'd also say that the following should be equivalent: a.encloses(b) and a.intersection(b) == b

Essentially: Either it should be true only when no sides touch, or it should allow any side to touch, an alternative would be to add an encloses_inclusive or some other wording that does what this does, and change all the checks for the encloses to be strict

(Will open a godot-cpp equivalent if this gets approved)

This differs from `Rect2(i)` and was fixed for those classes in the past
@AThousandShips AThousandShips added bug topic:core cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 12, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Jan 12, 2024
@AThousandShips AThousandShips requested review from a team as code owners January 12, 2024 17:01
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Yup, this is good for consistency and I'd say matches user expectations. Approved.

@akien-mga
Copy link
Member

Would like @lawnjelly's opinion on this, I remember a lot of bikeshedding last time we attempted changing such conventions.

@akien-mga akien-mga removed cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jan 13, 2024
@Sauermann
Copy link
Contributor

The bikeshedding in #57469 was mostly about expand_to with the result, that it works as intended.
My understanding was, that the change to a1.encloses(a1) == true was the consensus. So this PR simply fixes a bug in my opinion.

@lawnjelly
Copy link
Member

Yes this is fine imo. The difficult version is the integer bound where you can have exclusive or inclusive definition at upper border. 👍

@AThousandShips
Copy link
Member Author

AThousandShips commented Jan 14, 2024

Will add some extra checks like these to the Rect2(i) in a separate PR to ensure compliance

And I'll open a stripped down 3.x one without the tests

@akien-mga akien-mga merged commit f39baf0 into godotengine:master Jan 15, 2024
15 checks passed
@AThousandShips AThousandShips deleted the aabb_fix branch January 15, 2024 12:37
@AThousandShips AThousandShips removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jan 15, 2024
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
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.

6 participants