RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.32k stars 1.26k forks source link

[geometry] Standardize the description of field variables related to hydroelastic contact surfaces. #15616

Closed DamrongGuoy closed 3 years ago

DamrongGuoy commented 3 years ago

Requirement Specification

Introduction

This is a request to standardize the description string of field variables returned by the function geometry::MeshFieldLinear::name(). It's used in hydroelastic contact surfaces. It's used in the pressure fields of compliant geometries. Right now it inconsistently returns "e" or "pressure" or "pressure(Pa)" depending on the code that generates the MeshFieldLinear.

When I am preparing slides for the future OKR Review, I got the problem that Paraview treats the pressure field from the box and the pressure field from the cylinder separately. They have their separated color bars, so I cannot even say a color "white" maps to which color bars [0, 1e5] Pa or [0, 5e4] Pa. Is white 25000 Pa or 50000 Pa? This is how it looks:

box_cylinder_field_intersection

The next section gives details for contact surfaces; then, the next-next section gives details for compliant geometries.

This requirement specification also includes a proposal to improve the situation. Ideally, in hydroelastics, we should have the description as "pressure (Pa)" meaning that it is a pressure field with SI unit of Pascal. However, for other applications, we should allow other descriptions as well. The proposal is just an idea, which could be different from the actual PR.

Note:

Currently name() has no effect on the simulation. It is used for debugging and used in mesh_to_vtk for visually inspecting the field variables and create presentations in Paraview. It is not passed to drake_visualizer, meshcat, or rviz. It might in the future.

Contact Surface

The function geometry::ContactSurface::e_MN() returns a SurfaceMeshFieldLinear, whose function name() returns a string description of the field variable defined on the contact surface.

Right now it is set by these code:

Compliant Geometries

Compliant geometries use VolumeMeshFieldLinear, whose function name() returns a string description of the hydroelastic pressure field inside their volume.

Right now it is set by these code:

A Possible Solution 1.

Perhaps we can define a const string somewhere like this:

const std::string kPressureFieldDescription{"pressure (Pa)"};

and change all the above code to use it.

For other applications like "deformable", we might ask the author to define a similar string constant.

I'm not sure where the string constants should reside, perhaps somewhere near MeshFieldLinear code?

A Possible Solution 2.

Perhaps we can define a const string somewhere like this:

const std::string kPressureFieldDescription{"pressure (Pa)"};

and change all the compliant geometries (box, capsule, cylinder, ellipsoid, sphere) to use it.

For computing contact surfaces, we could construct the ContactSurface::e_MN().name_ from its source VolumeMeshFieldLinear::name(). This has the drawback that mesh_half_space_intersection does not take a source VolumeMeshFieldLinear, so it will have to use kPressureFieldDescription directly.

For other applications like "deformable", we might ask the author to define a similar string constant.

A Possible Solution 3.

Remove the name member variable. Demand users of mesh_to_vtk to explicitly provide consistent field descriptions by themselves as another parameter to WriteMeshFieldLinearToVtk().

Special Request

I would like to have a common decision on this issue, so I can use it in the coming compliant-compliant hydroelastic contact surfaces.

DamrongGuoy commented 3 years ago

CC: @EricCousineau-TRI , @amcastro-tri, @SeanCurtis-TRI , @rpoyner-tri , @joemasterjohn , @xuchenhan-tri for comments.

For now, assign to @rpoyner-tri to propose a solution.

amcastro-tri commented 3 years ago

I personally like "Solution 2", where contact surfaces inherit the name from their source. With regards to mesh_half_space_intersection, could those methods take an additional string with the name of the source? Within compliant hydro code we can simply pass kPressureFieldDescription, while other callers like say "deformable" would pass their own specific string.

SeanCurtis-TRI commented 3 years ago

Here's a wild and crazy alternative: delete the name entirely.

I think the field name was another example of speculative coding not living up to the promise. It really only makes sense if you have a vector of fields and you need to distinguish they by name. As it is, every field we have is given a clear name as a member of a struct/class or parameter. As far as I'm aware, we have no code that actually depends on or even cares about the string associated with the data. Shoving a string into the field seems to provide no value.

amcastro-tri commented 3 years ago

ah, I like that idea much better. +1 to @SeanCurtis-TRI's wild crazy idea.

DamrongGuoy commented 3 years ago

This is the code that depends on it.

void WriteVolumeMeshFieldLinearToVtk(
    const std::string& file_name,
    const VolumeMeshFieldLinear<double, double>& field,
    const std::string& title);

When I make OKR presentation, I have a problem that Paraview will treat different fields with different names (pressure vs. pressure(Pa)) in different ways.

I have updated the Requirement Specification with more explanation and a picture to make this point clearer. PTAL.

In my opinion, names and units are very important. NASA once lost a spacecraft because of the wrong units.

SeanCurtis-TRI commented 3 years ago

So many thoughts:

  1. The function is already asking for a rather arbitrary "title". Why not ask for a VTK-friendly field name? Why not use the title itself? The caller knows what kind of field they have and can provide any label necessary.
  2. Names and units are important. Our fields are all named and documented as members of classes and function parameters. Sadly, string values will not stop us from blowing up rockets. It is not a mechanism that resolves the issue of units.

The reason this issue exists at all is because you've observed how easy it is for this particular string to become meaningless. It will bit rot because it's value can be arbitrary and no calculation will care. We have no tests that care. We could attempt to shore it up by documenting difficult-to-enforce standards and introduce a slew of unit tests, but the putative value doesn't seem to justify the cost.

You've convinced me we have a Paraview interface problem. The solution isn't to put something into the depths of Drake. It's to fix the interface.

(FTR, given the underlying philosophy of VTK and Paraview, I'd be equally willing to believe that there's a 100% Paraview-side solution to this as well.)

DamrongGuoy commented 3 years ago

@SeanCurtis-TRI, thank you. You have a good point that we can fix the problem from Paraview side as well. I'll update the requirement specification later.

amcastro-tri commented 3 years ago

I agree writing VTK files is a different issue, separate from Drake internals. FYI, something like this would satisfy most (all?) of our needs for debugging @DamrongGuoy?: https://mjmorse.com/blog/vtk-writer/ The API looks nice and it allows you to name fields separately and even write several ones into a single file.

DamrongGuoy commented 3 years ago

Thanks for telling me about vtk-writer. I'm not aware of it. I'll let @rpoyner-tri the assignee of this issue check it out.

rpoyner-tri commented 3 years ago

First, +1 to Sean's idea of removing the names. Second, it seems there is certainly some way to fix names on the Paraview side, since it is python-scriptable. For example, there is this somewhat related issue. Third, the file format emitted by mesh_to_vtk.{h,cc} is ASCII, so in the short run, we could repair data files directly using a text editor or even sed.

So, we have a number of possible responses available. Here are a few that come to mind:

For the paper-writing season, I would recommend the "do nothing in Drake" option. After that, we can maybe revisit.

DamrongGuoy commented 3 years ago

@rpoyner-tri . Thank you for the ideas and the pointer to that Kitware issue.

I let you schedule the work freely after the mid-September paper deadline.