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

[doc,geometry,multibody] Add refs to Hydroelastic User Guide #16549

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

rpoyner-tri
Copy link
Contributor

@rpoyner-tri rpoyner-tri commented Feb 8, 2022

Closes: #15743

The module ... it is written.


This change is Reviewable

@rpoyner-tri rpoyner-tri added the release notes: none This pull request should not be mentioned in the release notes label Feb 8, 2022
Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

+@DamrongGuoy for feature review.
+(release notes: none)

Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

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

There're lint errors, but I'll start reviewing, anyway.

Reviewed all commit messages.
Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @rpoyner-tri)

a discussion (no related file):
Since proximity_properties.h declares its content in internal, I suppose it won't show up in Doxygen, right?



multibody/parsing/parsing_doxygen.h, line 388 at r1 (raw file):

@see drake::geometry::ProximityProperties,
@ref mbp_hydroelastic_materials_properties "Hydroelastic contact",

Now that we have hug_properties in the next line, do we still want "Hydroelastic contact" here? I would think hug_properties is more direct to the point.

Optional. Remove it?

ref mbp_hydroelastic_materials_properties "Hydroelastic contact",

I'm fine with keeping it too.

Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @rpoyner-tri)

a discussion (no related file):
I used bazel run //doc/doxygen_cxx:build -- --serve --quick drake.{geometry,multibody}, and it looked good.


Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @rpoyner-tri)

a discussion (no related file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Since proximity_properties.h declares its content in internal, I suppose it won't show up in Doxygen, right?

I meant the content where this PR changes.


Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @rpoyner-tri)

a discussion (no related file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

I meant the content where this PR changes.

There is no reference to @ref creating_hydro_reps or @ref hug_properties showing up on this web page.

image.png


Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @rpoyner-tri)

a discussion (no related file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

I used bazel run //doc/doxygen_cxx:build -- --serve --quick drake.{geometry,multibody}, and it looked good.

image.png


Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

a discussion (no related file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

There is no reference to @ref creating_hydro_reps or @ref hug_properties showing up on this web page.

image.png

You need to click through to the individual function descriptions on that page to see the references. For example:

Screenshot from 2022-02-08 18-25-07.png

There is one change within the internal:: namespace. I figure that's just a freebie for people reading the code directly, and a hedge against future copy-paste.


Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

+@xuchenhan-tri for platform review, Wednesday schedule.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform) (waiting on @DamrongGuoy and @xuchenhan-tri)

Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform) (waiting on @xuchenhan-tri)

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

You need to click through to the individual function descriptions on that page to see the references. For example:

Screenshot from 2022-02-08 18-25-07.png

There is one change within the internal:: namespace. I figure that's just a freebie for people reading the code directly, and a hedge against future copy-paste.

I see. Thanks for the explanation! Indeed I misunderstood which one is internal which one is not.


Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees DamrongGuoy,xuchenhan-tri(platform) (waiting on @rpoyner-tri)


geometry/proximity_properties.h, line 57 at r2 (raw file):

       - accounting for differences between tessellated meshes and half spaces.

 For the full discussion of preparing geometry for use in the

BTW, odd line break.


geometry/proximity_properties.h, line 130 at r2 (raw file):

/** Overload, intended for shapes that don't get tessellated in their
 hydroelastic representation (e.g., HalfSpace and Mesh).
 See @ref hug_properties.  */

BTW, is "hug" short for "hydro user guide"? Took me a long while to figure that out.

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees DamrongGuoy,xuchenhan-tri(platform) (waiting on @DamrongGuoy and @xuchenhan-tri)


geometry/proximity_properties.h, line 130 at r2 (raw file):

Previously, xuchenhan-tri wrote…

BTW, is "hug" short for "hydro user guide"? Took me a long while to figure that out.

yeahh. :)

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees DamrongGuoy,xuchenhan-tri(platform) (waiting on @rpoyner-tri)

@xuchenhan-tri xuchenhan-tri merged commit ffa7f59 into RobotLocomotion:master Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the documentation on resolution_hint
3 participants