Skip to content

Fix ContactHalfSpace decoration to lie within the contact geometry. #2212

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

Merged
merged 3 commits into from
Jul 16, 2018

Conversation

chrisdembia
Copy link
Member

Fixes #2211

Brief summary of changes

  • This PR fixes the transform for ContactHalfSpace so that the half-space's decorative geometry does not protrode outside of the contact geometry (half in, half out).

Testing I've completed

  • I temporarily turned the visualizer on in testContactGeometry and ensured that the entire DecorativeBrick for the ContactHalfSpace appeared below ground.
  • I ran and fixed testVisualization (this test initially failed after I changed ContactHalfSpace.cpp).

CHANGELOG.md (choose one)

  • no need to update because...our current way of displaying ContactHalfSpace was not in any previous release.

Copy link
Member

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

Would this need adjustment based on whether
contact is on top/bottom or is contact always on top and transform changes accordingly?

@chrisdembia
Copy link
Member Author

Would this need adjustment based on whether
contact is on top/bottom

No.

The change I made is correct regardless of the orientation of the contact half-plane.

Copy link
Member

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

LGTM. Ideally all these hardcoded numbers would be scaled by some measure of model size but that's outside the scope of this PR.

@chrisdembia
Copy link
Member Author

Thanks @aymanhab

Copy link
Member

@aseth1 aseth1 left a comment

Choose a reason for hiding this comment

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

LGTM. I only suggest making the brick even thinner so there is no issue trying to interpret what its thickness means.

@@ -77,8 +77,14 @@ void ContactHalfSpace::generateDecorations(bool fixed, const ModelDisplayHints&
const auto& X_BF = getFrame().findTransformInBaseFrame();
const auto& X_FP = getTransform();
const auto X_BP = X_BF * X_FP;
geometry.push_back(SimTK::DecorativeBrick(Vec3{.005, 0.5, 0.5})
.setTransform(X_BP).setScale(1)
const double brickHalfThickness = 0.005;
Copy link
Member

Choose a reason for hiding this comment

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

The issue here is that we are representing a plane with a brick. I think it is misleading to present a plane with a block with thickness. While I agree using the top surface of the brick is better (than the middle) in reality we shouldn't be presenting any thickness. Ideally we could draw planes. For the time being we could use a thickness of 1mm instead of 1cm?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed the thickness to 1mm. Here's a comparison:

1cm:

screen shot 2018-07-13 at 11 35 00 am

1mm:

screen shot 2018-07-13 at 11 34 15 am

Copy link
Member

Choose a reason for hiding this comment

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

👍 Looks more plane like to me.

Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to update testVisualization!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right. Updated.

Copy link
Member

@tkuchida tkuchida left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@tkuchida
Copy link
Member

@aseth1's only suggestion was implemented and the AppVeyor failure has already been noted in #2217.

@tkuchida tkuchida merged commit 07c1a67 into master Jul 16, 2018
@tkuchida tkuchida deleted the fix_ContactHalfSpace_geometry branch July 16, 2018 09:51
@chrisdembia
Copy link
Member Author

Thanks @tkuchida

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.

Height of ContactHalfSpace geometry is incorrect.
4 participants