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

Actually initialize zero_acceleration_kinematics_placeholder_ to zero #22020

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

Conversation

Brian-Acosta
Copy link
Contributor

@Brian-Acosta Brian-Acosta commented Oct 10, 2024

This comment in multibody_plant.h is lying about zero_acceleration_kinematics_placeholder_ getting populated with zeros :

  // When use_sampled_output_ports_ is true, then during Finalize() we populate
  // this with all-zero data, for use when the State has not yet been stepped.

This PR fixes this issue to avoid sending NaNs from the body_spatial_accelerations output port on the first step of a discret-time MBP simulation.


This change is Reviewable

@sherm1
Copy link
Member

sherm1 commented Oct 10, 2024

@drake-jenkins-bot ok to test

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

+@jwnimmer-tri for a quick (4 line) sampled-output-ports-related feature review, please. Is this a one-off or are there other instances we should fix?

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @Brian-Acosta)

@jwnimmer-tri
Copy link
Collaborator

I am happy to review. But it won't be as quick as four lines -- I will also need to develop and push the missing unit test.

@jwnimmer-tri jwnimmer-tri added the release notes: fix This pull request contains fixes (no new features) label Oct 10, 2024
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

+@sherm1 for feature review, please.

(We'll assign a platform reviewer who is not me, later on.)

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Brian-Acosta)

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Feature :lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Brian-Acosta)

@jwnimmer-tri
Copy link
Collaborator

+@ggould-tri for platform review per schedule (Tuesday), please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants