-
Notifications
You must be signed in to change notification settings - Fork 199
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
Conversation
39d6525
to
be8533d
Compare
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?) |
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 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. |
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? |
Let's make a decision on this as it's a perf improvement. As per the author's comments on Discord:
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. |
Thanks for the PR @b4l ; looks good except the non-robust impl for +1 for removing the |
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 |
There was a problem hiding this comment.
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.
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 |
The ahash MSRV issue was resolved upstream tkaitchuck/aHash#208 |
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. |
CHANGES.md
if knowledge of this change could be valuable to users.