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.

Sign up for GitHub

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

Jump to bottom

Made Body::containsPoint() consistent in surface points inclusion. #90

Closed
wants to merge 3 commits into from
Closed

Made Body::containsPoint() consistent in surface points inclusion. #90

wants to merge 3 commits into from

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Nov 20, 2018

No description provided.

@peci1 peci1 mentioned this pull request Nov 20, 2018
Closed
@peci1
Copy link
Contributor Author

peci1 commented Nov 20, 2018

See #91 for context.

@peci1 peci1 changed the title Added a (so far failing) test for consistency of point inclusion tests. Made Body::containsPoint() consistent in surface points inclusion. Jan 26, 2019
rhaschke
rhaschke requested changes Jan 26, 2019
View reviewed changes
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Thanks a lot. Some minor comments remain.

README.md Show resolved Hide resolved
test/test_point_inclusion.cpp Outdated Show resolved Hide resolved
test/test_point_inclusion.cpp Outdated
const double sq2 = sqrt(2) / 2;
double sq2_eps = sq2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here of course.

test/test_point_inclusion.cpp Outdated Show resolved Hide resolved
@peci1 peci1 force-pushed the consistent_point_inclusion branch 2 times, most recently from 0b698b4 to 21b060e Compare January 26, 2019 01:57
rhaschke
rhaschke requested changes Jan 27, 2019
View reviewed changes
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Looks good to me. One final request: Could you please squash the last two commits (bug fix + refactoring) into one. Then I propose a merge-commit to keep the clean history of this PR branch.

@peci1
Copy link
Contributor Author

peci1 commented Jan 27, 2019

I squashed and force-pushed. I didn't quite get the part about merge commit - there seems to be nothing to merge... Or did you mean merge instead of force-push?

rhaschke
rhaschke requested changes Jan 27, 2019
View reviewed changes
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

One more nitpick.

test/test_point_inclusion.cpp
@@ -360,8 +373,10 @@ TEST(CylinderPointContainment, Basic)

// near two-axis maximum
const double sq2 = sqrt(2) / 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like sq2 is only used in a single location. Do you mind to use sq2_eps for this location too, thus allowing to rename s2_eps back to sq2. This way, the bugfix wouldn't require too many changes for the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually used at 2 locations: line 400 and 406. I wanted to use this "clean" value where it works, and only use sq2_eps at places which are really affected by IEEE 754 rounding errors. If you just don't like the name sq2_eps, I can try finding a new one :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I don't like the name very much. But it don't have a better idea either. I'm not so strong on names ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed sq2_eps to sq2e. Also, to keep consistency throughout the code, I renamed sq2 to sq2e in the sphere test.

@rhaschke
Copy link
Contributor

I didn't quite get the part about merge commit - there seems to be nothing to merge.

To merge this PR branch into its parent, github provides several options: explicit merge commit, rebase, or squash-rebase. To keep the history of your 3 commits (thanks a lot cleaning them up), I suggested the merge commit.

@peci1
Copy link
Contributor Author

peci1 commented Jan 27, 2019

Hmm, is this something new on github? I don't see any such setting in the PR options from my side. It looks to me like options for the one who is finally merging this PR, which is not me (I don't have commit access). Or do I miss a point? Should I create an explicit merge commit in my repo and then send a PR from my indigo-devel to upstream?

@rhaschke
Copy link
Contributor

Hope this helps to explain the different policies.
You are right, only maintainers with push access see the mentioned merge options.
No other action than #90 (comment) is required for you.

@peci1
Copy link
Contributor Author

peci1 commented Jan 27, 2019

Ok, got it. There's still an unanswered question in #90 (comment) .

@rhaschke
Copy link
Contributor

There's still an unanswered question in #90 (comment) .

Sorry. Didn't noticed this one. Answered now.

rhaschke
rhaschke reviewed Jan 28, 2019
View reviewed changes
test/test_point_inclusion.cpp Outdated
EXPECT_TRUE( cylinder.containsPoint(Eigen::Vector3d(0.70, 0.70, 0.00)));
EXPECT_SURF( cylinder.containsPoint(Eigen::Vector3d(sq2 , sq2 , 0.00)));
EXPECT_SURF(cylinder.containsPoint(Eigen::Vector3d(sq2e , sq2e , 0)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Better. Could you also restore the previous fixed number format: 0 -> 0.00 and clean spaces, commas, etc.
Your original code looked better in this regard. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

rhaschke
rhaschke approved these changes Jan 28, 2019
View reviewed changes
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Great! Thanks for your patience.

@rhaschke rhaschke requested a review from v4hn January 28, 2019 21:29
davetcoleman
davetcoleman approved these changes Jan 29, 2019
View reviewed changes
README.md Show resolved Hide resolved
@peci1 peci1 mentioned this pull request Feb 21, 2019
Merged
@rhaschke
Copy link
Contributor

Closing this in favor of #97.

@rhaschke rhaschke closed this Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davetcoleman davetcoleman davetcoleman approved these changes

@rhaschke rhaschke rhaschke approved these changes

@v4hn v4hn Awaiting requested review from v4hn

Assignees
No one assigned
Labels
awaits 2nd approval
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@peci1 @rhaschke @davetcoleman