-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[fem] Refactor ForceDensityField #23033
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
[fem] Refactor ForceDensityField #23033
Conversation
5698c57
to
153d805
Compare
There was a problem hiding this 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.
Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW lint is unhappy with point_source_force_field.h, causing CI failures.
First pass done. A couple of thoughts:
- It seems odd to have the Impl class derived from the nice-named class. Consider renaming like this:
ForceDensityField -> ForceDensityFieldBase
ForceDensityFieldImpl -> ForceDensityField
Then concrete implementations can continue to derive from ForceDensityField. - I can't tell if there is a problem with Clone() now. There are data members in both the base and impl classes, but the impl class doesn't do anything about cloning. Presumably this is left to the concrete classes. Are there unit tests somewhere that can show that those DoClone()s actually work properly with the new intermediate class stuck in there?
Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers
multibody/tree/force_density_field_impl.h
line 112 at r1 (raw file):
const internal::MultibodyTreeSystem<T>* tree_system_{nullptr}; ForceDensityType density_type_{ForceDensityType::kPerCurrentVolume};
BTW this field density_type_
is also present in the now-base-class ForceDensityField. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new unit test file with some new tests in r2. I'll push the name change in r3.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)
multibody/tree/force_density_field_impl.h
line 112 at r1 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW this field
density_type_
is also present in the now-base-class ForceDensityField. Is that intentional?
No! Removed, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed the name change in r3, but I'm not a huge fan of the new names. The name exposed in public APIs is now ForceDensityFieldBase
(e.g. in DeformableModel::AddExternalForce
)
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ForceDensityField also has public APIs so it's not obvious to me that there's a perfect solution. I do like how this looks in your test case:
class ConstantForceDensityField final : public ForceDensityField<double>
+@ggould-tri for platform review per rotation, please
Reviewed 22 of 22 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee ggould-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)
@drake-jenkins-bot mac-arm-sonoma-clang-bazel-experimental-release please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 18 files at r1, 22 of 22 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)
multibody/plant/deformable_model.cc
line 588 at r2 (raw file):
force_densities_) { /* We know that the static cast is safe because all concrete force density field is derived from ForceDensityField. */
typo: Grammar
Suggestion:
/* We know that the static cast is safe because all concrete force density
fields are derived from ForceDensityField. */
multibody/fem/force_density_field_base.h
line 38 at r2 (raw file):
class ForceDensityFieldBase { public: virtual ~ForceDensityFieldBase() = 0;
minor: I'm not sure what you are trying to accomplish by pure-virtualing this in the header and then setting it default in the cc file?
The goal of the commit is the remove the dependency of the fem library from the tree library so that later we create deformable multibody element (in the tree library) that depends on fem.
8f02c60
to
89cc4ab
Compare
There was a problem hiding this 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: 1 unresolved discussion
There was a problem hiding this 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
multibody/plant/deformable_model.cc
line 588 at r2 (raw file):
Previously, ggould-tri wrote…
typo: Grammar
Done
multibody/fem/force_density_field_base.h
line 38 at r2 (raw file):
Previously, ggould-tri wrote…
minor: I'm not sure what you are trying to accomplish by pure-virtualing this in the header and then setting it default in the cc file?
Pure virtual to make this class abstract and setting its implementation as default because there's nothing special needed for the destructor. To be clear, a definition in the cc file is still required for a pure virtual desctructor.
The goal of the commit is the remove the dependency of the fem library from the tree library so that later we create deformable multibody element (in the tree library) that depends on fem.
In particular, add a ForceDensityFieldImpl class so that the interface ForceDensityField no longer depends on tree and can be left in the fem library.
Towards #23029
This change is