[geometry] Streamline GeometryProperties interface

[SeanCurtis-TRI]

Problem

GeometryProperties was designed in such a way to allow third parties that need to associate arbitrary properties with geometries to support arbitrary downstream consumers of geometries. This led to the ("group name", "property") property identifier and the abstract values. The property identifiers allow the use of intuitive property names like "color", but can disambiguate them by selecting a unique group. However, this makes working with properties somewhat cumbersome.

1. Making sure the correct (group, property) pair is produced can be brittle. This is strictly a run-time check and typos can end up inadvertently hiding data.
2. The interface for accessing/setting properties becomes larger and more unwieldy (two parameters for each parameter).
3. Drake has a number of properties that it requires and get coordinate between various entities (e.g., parsing and MultibodyPlant). They pay a similar cost.
4. Accessing a property requires actively coordinating two strings and places the burden on the caller.

This gave rise to functionality found in proximity_properties.h. It's a "collection of data types and functions to help manage defining properties for geometries with the proximity role." This works with properties that we know some of Drake's built-in systems need -- frequently populated from parsing and accessed by downstream systems. However, it has some critical internal:: data and provides an interface that does not extend well. This is illustrated well in a recent PR.

We need a better way to accomplish the same end:

• Facilitate accessing/setting properties.
• Facilitate coordination of Drake's "canonical" properties.

This is important to resolve sooner than later. As we plan on embedding more geometry properties in SDF/URDF files, we want to have a nicely extensible, reasonable way to deal with all of these new properties coherently.

Updated Proposal

Updated based on discussion; see below for the original proposal.

The proposal has two orthogonal components. PropertyName and modifications to GeometryProperties derivations.

PropertyName

Introduce the class:

class PropertyName {
 public:
  PropertyName(std::string group, std::string property) : group_(std::move(group)), property_(std::move(property)) {}
  const std::string& group() const { return group_; }
  const std::string& property() const { return property_; }
 private:
  std::string group_;
  std::string property_;
};

and evolve the GeometryProperties API (using AddProperty as a single example).

// Old API
void AddProperty(const std::string& group, const std::string& property, ValueType value);
// New API
GeometryProperties& Add(const PropertyName& property, ValueType value);

Extending the API on GeometryProperties derivations.

Turn invocations like this:

ProximityProperties props;
AddContactMaterial(1e8, 0.25,  CoulombFriction<double>(mu_s, mu_d), &props);

into this

ProximityProperties props;
props.Add(props.material_elastic_modulus(), 1e8)
    .Add(props.material_dissipation(), 0.25)
    .Add(props.material_coulomb_friction(), CoulombFriction<double>(mu_s, mu_d));

The latter API is infinitely extensible and all property values have clear semantics.

With the following design:

In a derivation of GeometryProperties (e.g., ProximityProperties), there will be a set of canonical properties. For a canonical property foo in group bar, we introduce a static method to return its PropertyName. This method is used to to reference canonical properties (as shown above).

static const PropertyName& bar_foo();

Original Proposal

The proposal has two orthogonal components. PropertyName and modifications to GeometryProperties derivations.

PropertyName

Introduce the class:

class PropertyName {
 public:
  PropertyName(std::string group, std::string property) : group_(std::move(group)), property_(std::move(property)) {}
  const std::string& group() const { return group_; }
  const std::string& property() const { return property_; }
 private:
  std::string group_;
  std::string property_;
};

and evolve the GeometryProperties API (using AddProperty as a single example).

// Old API
void AddProperty(const std::string& group, const std::string& property, ValueType value);
// New API
void AddProperty(const PropertyName& property, ValueType value);

Extending the API on GeometryProperties derivations.

Turn invocations like this:

ProximityProperties props;
AddContactMaterial(1e8, 0.25,  CoulombFriction<double>(mu_s, mu_d), &props);

into this

ProximityProperties props;
props.set_elastic_modulus(1e8).set_dissipation(0.25),set_friction({mu_s, mu_d});

With the following design:

In a derivation of GeometryProperties (e.g., ProximityProperties), there will be a set of canonical properties. For a canonical property foo with type T we introduce three methods:

// Sets the value of [description of foo]. Equivalent calling ProximityProperties::UpdateValue(PropertyName("foos_group", "foos_name"), value);.
ProximityProperties& set_foo(T value);
// Gets the value of [description of foo]. Returns nullptr if the property hasn't been defined.
const T* foo() const;
// Returns the Propertyname for [description of foo].
PropertyName foo_name() const;

Notes:

1. I'm open to whether set_foo() is really just non-const foo(value) for compactness.
2. I'm also open to discussion on the semantics of the getter. Should it throw?

[SeanCurtis-TRI]

cc @joemasterjohn (from our conversation) cc @EricCousineau-TRI (as part of this arises from things we've discussed in the past)

[EricCousineau-TRI]

I like the new API for ProximityProperties!

I'm not sure how I feel about the usage of overload sugar for extending properties; instead, I'd rather we expose the coordinates? e.g. instead of

ProximityProperties props;
props.set_elastic_modulus(1e8).set_dissipation(0.25),set_friction({mu_s, mu_d});

instead do something like this?

ProximityProperties props;
props  // BR
   .set(kPropertyElasticModulus, 1e8)
   .set(kPropertyDissipation, 0.25)
   .set(kPropertyFriction, ({mu_s, mu_d});

[SeanCurtis-TRI]

@EricCousineau-TRI I see what you're saying. How about a middle ground?

ProximityProperties props;
props  // BR
    .UpdateProperty(props.FooName(), foo_value)
    .UpdateProperty(props.BarName(), bar_value);

1. In your example, the set() method is just a copy of UpdateProperty() so it serves no purpose.
2. I've replaced kFoo with a well-scoped props.FooName() and keeps things tighter. If kFoo existed independently of ProximityProperties it could be used with IllustrationProperties. I think it would be harder to make that mistake the names were scoped by the property type. (Because kFoo would be of type PropertyName we'd scope it either via namespace or "ownership".)

ADDENDUM

So, what we're really talking about here is simply changing the AddProperty, UpdateProperty to return a reference to self instead of void. (And shortening the specification of the property.)

[EricCousineau-TRI]

Looks better!

Two q's:

• Do .FooName(), .BarName() need to be instance methods? Or can they be static? (or does it get more verbose that way?)
• Why even have the trailing name *Property, if the container only handles properties? Why not just Add(...), Update(...) (just to save on real estate)

[sherm1]

The original purpose of (group, name) as you stated above was so that a name (like "color") could be used for several purposes. Doesn't this design defeat that?

ProximityProperties props;
props.set_elastic_modulus(1e8).set_dissipation(0.25),set_friction({mu_s, mu_d});

Now the name "dissipation" is locked under the covers to a particular group. But that is a generic name like "color" and could be a property for a variety of different models. Similarly "friction" might be parameterized differently for different models but here it looks like there is a preferred group?

[SeanCurtis-TRI]

@EricCousineau-TRI

• I agree 100% on the *Property. The new API should simply be Add(), Update(), Remove(). More aggressively, how about HasProperty() --> Contains()? Etc.
• In my mind they would definitely be static. But the invocation props.FooName() will still be valid (and I'd end up preferring it because it's shorter than ProximityProperties::FooName(). :)

@sherm1

• A property is still uniquely defined by a group and property name. The only difference is that instead of the caller being responsible for coordinating the two strings, they create a PropertyName which formalizes the pairing of the two.
• The "hard-coded" names are only those very specific properties that we use to facilitate properties that Drake systems are known to need. This is no way eliminates the ability to stash arbitrary other properties in. For example, (using "color" as an example). We have ("phong", "diffuse") as a Drake-canonical property. It would get a method akin to what's been discussed. But someone could always do props.Add({"my_group", "diffuse", Vector4d{1, 1, 1, 1}); and that would be a unique property in the set that they would access via their own stored PropertyName -- there just wouldn't be any sugar for the non-canonical property.

[sherm1]

In that case how about having the Drake-canonical names contain both the group and property name, like set_phong_diffuse()? That still has the advantage of compile-time enforcement but doesn't gloss over the fact that all properties are identified with group/name components.

[SeanCurtis-TRI]

@sherm1 I'm definitely open to that. It just comes down to figuring out what "Foo" should be. Although, as @EricCousineau-TRI proposed, we wouldn't have: props.set_phong_diffuse(diffuse).set_phong_specular(specular); Instead, we'd have: props.Add(props.PhongDiffuseName(), diffuse).Add(props.PhongSpecularName(), specular);

@EricCousineau-TRI Here's a reason why we may want the explicit setters. For some of these properties, we know they have constraints (e.g., non-negative). By having explicit setters for the canonical properties, we can do value checking at the time the property is added rather than waiting for feedback when the value is accessed. That holds a great deal of appeal for me. Admittedly, that could be done under-the-hood -- provide a virtual ValidateOrThrow() method that sub-classes can implement so that the Add(name, value) gets funneled through a function that will validate an explicitly enumerated list of properties and let all others go through. Thoughts?

Edit: I'm strongly leaning towards the under-the-hood so that the properties we care about will get vetted 100% of the time.

[DamrongGuoy]

Is it too early to ask about backward compatibility? The client code using the current API will continue to work, right?

[SeanCurtis-TRI]

The old API will simply be deprecated.

[EricCousineau-TRI]

I'm strongly leaning towards the under-the-hood so that the properties we care about will get vetted 100% of the time.

I get that... but at this point, then, why even use AbstractValue stuff with GeometryProperties for builtin types? I guess similar to our discussion about CameraProperties, if you want more compile-time checks, then why even admit run-time slop?

Also, why should the specific setters get the nicety of checking, vs. the type-erased versions?

If you want to enforce properties in one entry point in an interface, ideally you should check it in all entry points?

[SeanCurtis-TRI]

why should the specific setters get the nicety of checking, vs. the type-erased versions?

That's why I favor the under-the-hood. So, that doesn't exist.

why even use AbstractValue stuff with GeometryProperties for builtin types

This is something I considered back when I wrote GeometryProperties in the first place. In fact, if you look at PerceptionProperties, I still have a TODO that says: "Should this have a render label built in?" I still don't have a strong opinion. That said, I do like the idea of having a uniform interface for setting properties. So, I'm inclined to stick with the "everything is a property" approach and merely give derived classes the option to vet parameters when they come in.

[EricCousineau-TRI]

Ah, yeah, sorry for missing that. But then, it's back to that kinda awkward "I'm doing static checking on dynamic values"... but yeah, if you're up for it, perhaps while keeping the TODO's you mentioned, then :shrug: It feels more complex then it needs to be, but if there is a point at which having uniform type-erased access to all (group, property) pairs (e.g. for RPC rendering), then I could see that merited. (Though still, it feels like perhaps it should be a transformation as a whole, rather than transforming properties as they come in...)

[SeanCurtis-TRI]

...that kind awkward "I'm doing static checking on dynamic values"

Surely, "Doing dynamic checks on static values"?

but if there is a point at which having uniform type-erased access to all (group, property) pairs...

There absolutely is, built into the design of how GeometryProperties relates to downstream consumers (this occurred to me this morning). Default values. Downstream property consumers can arbitrarily apply their own rules if a property is undefined. To have undefined, each "hard-coded" property would have to support such a state (e.g., std::optional), so we'd be doing runtime checks on every property anyways.

And throw RPC on top of that, having a single interface for all property values keeps things simple.

[SeanCurtis-TRI]

@EricCousineau-TRI @sherm1 @DamrongGuoy @joemasterjohn

All of you have weighed in on this to one degree or another. I've posted a draft PR (#13742) and would love to solicit feedback.

[amcastro-tri]

can someone (@SeanCurtis-TRI or @EricCousineau-TRI) explain to me the difference between: props.set_phong_diffuse(diffuse).set_phong_specular(specular); and props.Add(props.PhongDiffuseName(), diffuse).Add(props.PhongSpecularName(), specular);?

I personally like the first one better. Do you guys like the second one because it's your personal preference or is there a darker design requirement I am missing?

Another question, feel free to skip if no relevant. Would this still be compatible with the f2f ideas you gave us on how to extend the API to support things like AutoDiffXd parameters?

[SeanCurtis-TRI]

@amcastro-tri Good questions:

• props.set_foo(value) vs props.Add(props.foo(), value)

  • The primary reason for this change comes from the original proposal.

  • For each canonical property, we required three new methods (a getter, a setter, and a name).

  • Using the Add and Update methods that (more or less) already existed, means that all we need to do (to the public API) is add the name. It maximizes re-use.

  • Furthermore, the set_foo() and foo() methods would be redundant to the already existing API. If I can call set_foo() and update(props.foo_name()) it invites the question, are they different? Do I have to be careful about which I call when? The answer is "no", but why invite the question?

  • Admittedly, the Add(props.NameOfThing(), value) isn't quite as compact as set_name_of_thing(value) spelling, but that's really its only real value -- the ability to shave off a couple of characters (and it is literally just a couple of characters in savings). And now that I've written the spelling with Add(props.NameOfThing(), value) all over Drake code (see the linked PR), I'm quite content with how it looks.

  • Transmogrification (although I don't want to get too deep into this topic in this issue)

  • As I consider a way to handle scalar-dependent properties, the Add() and Update() API is going to be far more compatible with those efforts than a massive, type-dependent collection of ad hoc methods. So, my initial judgement is the Add() Update() api will be less limiting than the set_foo() api would be.

[amcastro-tri]

... spelling, but that's really its only real value

sry, but that's what I still don't see. What is the additional value? I am not advocating yet for one or the other, but thus far I don't have any additional arguments more than just a stylistic choice.

[SeanCurtis-TRI]

Now I'm confused. I don't think the comment you quoted successfully communicated what I'd intended to communicate.

1. I understand that you prefer set_foo() to Add(props.foo()).
2. The first four bullet points are why Add(props.foo()) is preferred over set_foo(). (Well, the first sub-bullet isn't really an argument one way or the other, but bullets 2-4 are.)
3. The fifth sub-bullet (which you quoted from), indicates the only putative benefit of set_foo() over Add(props.foo()) and argues that that benefit is slight at best.

Therefore, for strongly quantitative reasons, Add(props.foo()) should be preferred over set_foo(). It obviously doesn't address the qualitative reasons of preferred style, but in code, I'd argue that substance should win over style if they are mutually exclusive.

[amcastro-tri]

sry, I'll ask you in another venue. To me the bullets points 2-4 read more like specifications after a decision was made, not like an argument. Definitely I am not parsing this right.

[RussTedrake]

Can I add to the list the fact that the varios GeometryProperties seem to not be documented (that I can find) in doxygen? The only way that I've been able to find out which properties to set is by crawling through the code...

Tonight example was reading the nice documentation for RenderLabel, but not being able to find anywhere (til I dug into the code) how/where I would set the RenderLabel. I guessed it to be the PerceptionProperties, once I stumbled upon that when looking at the likely places, but those classes are not documented?

[SeanCurtis-TRI]

As I transfer ownership of this issue, I wanted to make my (now stale) WIP branch available. If I get extra times, I'll see if I can't rebase it on a more current version of master. No guarantees.

The branch is here: https://github.com/SeanCurtis-TRI/drake/tree/PR_streamline_property_interface_13688