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

Point in triangle and rect performance improvements #1057

Merged
merged 14 commits into from
Feb 23, 2024
Merged

Point in triangle and rect performance improvements #1057

merged 14 commits into from
Feb 23, 2024

Conversation

b4l
Copy link
Member

@b4l b4l commented Aug 24, 2023

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

@b4l b4l force-pushed the triangle branch 2 times, most recently from 39d6525 to be8533d Compare August 24, 2023 17:38
@urschrei
Copy link
Member

Thanks for the PR @b4l! Could you say a little about how this perf improvement works? Is it purely down to switching away from using Robust? Are there any drawbacks (e.g. precision issues?)

@b4l
Copy link
Member Author

b4l commented Aug 25, 2023

From my rather superficial understanding, the previous versions involved a conversion to a polygon, which allocates a vector that is considered expensive. Then subsequent, in the case of rect robust predicates are suboptimal as there is no interpolation in play since it is axis-aligned and a simple comparison is adequate. For the triangle cases, this implementation leverages ideas around barycentric coordinates and half-plane calculations, which are somewhat robust in terms of not involving any division, though presumably not as robust as robust predicates, as the name suggests. For the latter implementations based on robust predicates without allocation are commented out offering lesser performance improvements given the robustness trade-off.

Regarding precision issues, I have only a very basic understanding of the robustness and no idea of the implications this has in practice. The test /bench with the point on the triangle line works as expected though actually looking for reviewers' input regarding that.

@urschrei
Copy link
Member

Given the explanation (thank you!) and that it's passing tests currently, I'm inclined to merge this. Anyone else got input for or against?

@urschrei
Copy link
Member

Let's make a decision on this as it's a perf improvement. As per the author's comments on Discord:

I can not vouch for the robustness, though there is a version using robust predicates for the triangle case commented out which still offers major performance improvements compared to the existing implementation. so maybe we can use this one for confidence?

So the conservative solution is to go with the robust predicate if there's some doubt about robustness. Alternatively, someone dreams up some more tests.

@urschrei urschrei requested a review from rmanoka January 22, 2024 14:24
@rmanoka
Copy link
Contributor

rmanoka commented Jan 23, 2024

Thanks for the PR @b4l ; looks good except the non-robust impl for Triangle; let's use the slightly slower robust impl. on that too.

+1 for removing the Vec allocations though.

let ls = LineString::new(vec![self.0, self.1, self.2, self.0]);
use crate::utils::{coord_pos_relative_to_ring, CoordPos};
coord_pos_relative_to_ring(*coord, &ls) == CoordPos::Inside
// leverageing robust predicates
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go with this impl. if it's correct, rather than the non-robust one.

For quick intro. to robust, you may want to refer this example that explains why it's important.

@b4l
Copy link
Member Author

b4l commented Feb 19, 2024

Thanks for reviewing and sorry for the late response! I changed to the versions using robust predicates.

Should I remove the commented-out versions?

Edit: CI failure seems to be unrelated, see #1149

@frewsxcv
Copy link
Member

The ahash MSRV issue was resolved upstream tkaitchuck/aHash#208

@urschrei
Copy link
Member

Are we good to merge this before releasing? I know we have version control, but I'm personally OK with hanging on to the commented-out versions for now.

@frewsxcv frewsxcv added this pull request to the merge queue Feb 23, 2024
Merged via the queue into main with commit b1e550a Feb 23, 2024
15 checks passed
@frewsxcv frewsxcv deleted the triangle branch February 23, 2024 15:30
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.

None yet

4 participants