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

Add the Rect::overlaps and Rect::contains_rect methods #347

Merged
merged 6 commits into from
Aug 3, 2024

Conversation

nils-mathieu
Copy link
Contributor

I stumbled on this while toying with the vello rendering engine (which is great btw, you guys are doing god's work). I'm trying to create a simple audio sequencer with it and while trying to determine whether shapes were visible on screen, I found that there was no way of checking for the intersection of two Rects.

Right now, the only way to do that is to compute their intersection, then verify whether it's empty or not.

let rect1 = Rect::new(0.0, 0.0, 10.0, 10.0);
let rect2 = Rect::new(5.0, 5.0, 15.0, 15.0);
assert!(!rect1.intersect(rect2).is_empty());

This is not ideal for two reasons:

  1. It's not very ergonomic. Checking whether two rectangles overlap should probably not involve that intermediary step.
  2. This is doing a bunch of wasted work to find out what the intersection is when we only care about whether it's empty or not.

This pull request adds Rect::overlaps and Rect::contains_rect, which do exactly that.

let rect1 = Rect::new(0.0, 0.0, 10.0, 10.0);
let rect2 = Rect::new(5.0, 5.0, 15.0, 15.0);
assert!(rect1.overlaps(rect2));
assert!(!rect1.contains_rect(rect2));

@nils-mathieu nils-mathieu changed the title Add the Rect::intersect method Add the Rect::overlaps and Rect::contains_rect methods May 25, 2024
Copy link
Collaborator

@derekdreery derekdreery left a comment

Choose a reason for hiding this comment

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

I think this makes sense, and I've seen similar methods to these in other geom libraries.

src/rect.rs Outdated Show resolved Hide resolved
@nils-mathieu
Copy link
Contributor Author

nils-mathieu commented Jun 15, 2024

My mistake, I completely forgot about this. I just commited the suggested modifications.

@waywardmonkeys
Copy link
Contributor

@nils-mathieu Could you please rebase this on top of current main to pick up the CI changes / updates? Thanks!

@nils-mathieu
Copy link
Contributor Author

Github's autosync merged rather than rebasing without asking, I hope it's okay.
Seems green though x)

@waywardmonkeys
Copy link
Contributor

One last thing (from me): Could you add an entry to CHANGELOG.md? Just use the existing format, add the PR number sorted numerically and your GitHub user name sorted alphabetically and an entry for this under 'Added'.

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

I'm going to approve this, as the methods seem useful, but I will point out that overlaps is a weaker predicate than the question of whether there exists a point that is inside both rectangles, as the latter is defined as x0 <= x < x1. The documentation is correct, and, while I considered another name like "overlaps_inclusive," I think that's clunky and not worth it.

Thanks for the PR and sorry for the delay.

@nils-mathieu
Copy link
Contributor Author

done!

@waywardmonkeys
Copy link
Contributor

Thanks everyone!

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Aug 3, 2024
Merged via the queue into linebender:main with commit a568ab2 Aug 3, 2024
15 checks passed
@waywardmonkeys waywardmonkeys added this to the August, 2024 Release milestone Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants