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

isInCircleAdapt sketch Java port #311

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Komzpa
Copy link

@Komzpa Komzpa commented Sep 4, 2018

Do the non-robust calculation and calculation of error margin. If calculation is possibly not robust, fall back on slower math.

https://www.cs.cmu.edu/afs/cs/project/quake/public/code/predicates.c

References #298

Do the non-robust calculation and calculation of error margin. If calculation is possibly not robust, fall back on slower math.

https://www.cs.cmu.edu/afs/cs/project/quake/public/code/predicates.c
if (-det > errbound)) {
return false;
}
return isInCircleDDSlow(a, b, c, p);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should call inCircleDDFast - it is equivalent and faster

@dr-jts
Copy link
Contributor

dr-jts commented Sep 4, 2018

Need unit tests before rolling this in.

@dr-jts
Copy link
Contributor

dr-jts commented Sep 4, 2018

Good find. This should provide both performance and robustness.

The same approach is currently used in computing the orientationIndex. Also based on Shewchuk's fine work.

@Komzpa
Copy link
Author

Komzpa commented Sep 4, 2018

Just in case, I would be happy if someone familiar with java takes over this PR, to make proper formatting, unit tests and code style.

@dr-jts
Copy link
Contributor

dr-jts commented Sep 4, 2018

Makes sense. And add some unit tests. Not sure when I'll have time to work on this though.

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.

2 participants