Allow AssignRole(IllustrationProperties, kReplace) valid

This allows illustration properties to be replaced. It hasn't been
allowed previously because there is no formal connection between
changing those properties and guaranteeing they proppagate into
the visualizer.

This "addresses" that guarantee by printing a warning to the console
introducing a TODO for making it more robust.

bindings/pydrake/geometry_py.cc

@@ -758,8 +758,8 @@ void DoScalarIndependentDefinitions(py::module m) {
             [abstract_value_cls](Class& self, const std::string& group_name,
                 const std::string& name, py::object value) {
               py::object abstract = abstract_value_cls.attr("Make")(value);
-              self.AddPropertyAbstract(group_name, name,
-                  abstract.cast<const AbstractValue&>());
+              self.AddPropertyAbstract(
+                  group_name, name, abstract.cast<const AbstractValue&>());
             },
             py::arg("group_name"), py::arg("name"), py::arg("value"),
             cls_doc.AddProperty.doc)
@@ -768,8 +768,8 @@ void DoScalarIndependentDefinitions(py::module m) {
             [abstract_value_cls](Class& self, const std::string& group_name,
                 const std::string& name, py::object value) {
               py::object abstract = abstract_value_cls.attr("Make")(value);
-              self.UpdatePropertyAbstract(group_name, name,
-                  abstract.cast<const AbstractValue&>());
+              self.UpdatePropertyAbstract(
+                  group_name, name, abstract.cast<const AbstractValue&>());
             },
             py::arg("group_name"), py::arg("name"), py::arg("value"),
             cls_doc.UpdateProperty.doc)

bindings/pydrake/test/geometry_test.py

@@ -403,7 +403,7 @@ def test_geometry_properties_api(self):
             prop.GetProperty(group_name=default_group, name="to_update"), 17)
 
         prop.UpdateProperty(group_name=default_group, name="to_update",
-                         value=20)
+                            value=20)
         self.assertTrue(prop.HasProperty(group_name=default_group,
                                          name="to_update"))
         self.assertEqual(

geometry/geometry_properties.cc

@@ -52,11 +52,11 @@ void GeometryProperties::UpdatePropertyAbstract(const string& group_name,
         if (value_iter != group.end() &&
             group.at(name)->type_info() != value.type_info()) {
           throw std::logic_error(
-              fmt::format("AddProperty(): Trying to update property ('{}', '{}'"
-                          "); The property already exists and is of different "
-                          "type. New type {}, existing type {}",
-                          group_name, name, group.at(name)->GetNiceTypeName(),
-                          value.GetNiceTypeName()));
+              fmt::format("UpdateProperty(): Trying to update property ('{}', "
+                          "'{}'); The property already exists and is of "
+                          "different type. New type {}, existing type {}",
+                          group_name, name, value.GetNiceTypeName(),
+                          group.at(name)->GetNiceTypeName()));
         }
       });
 }

geometry/geometry_properties.h

@@ -266,8 +266,8 @@ class GeometryProperties {
 
   /** Updates the named property (`group_name`, `name`) with the given `value`.
    If the property doesn't already exist, it is equivalent to calling
-   `AddProperty`. If the property does exist, and its value the same type as
-   `value`, the property value will be updated.
+   `AddProperty`. If the property does exist, its value (which must have the
+   same type as `value`) will be replaced.
 
    @param group_name   The group name.
    @param name         The name of the property -- must be unique in the group.
@@ -281,7 +281,7 @@ class GeometryProperties {
     UpdatePropertyAbstract(group_name, name, Value(value));
   }
 
-  /** Adds the named property (`group_name`, `name`) with the giventype-erased
+  /** Adds the named property (`group_name`, `name`) with the given type-erased
    `value`. Adds the group if it doesn't already exist.
 
    @param group_name   The group name.
@@ -293,8 +293,8 @@ class GeometryProperties {
 
   /** Updates the named property (`group_name`, `name`) with the given
    type-erased `value`. If the property doesn't already exist, it is equivalent
-   to calling `AddPropertyAbstract`. If the property does exist, and its value
-   has the same type as `value`, the property value will be updated.
+   to calling `AddPropertyAbstract`. If the property does exist, its value
+   (which must have the same type as `value`) will be replaced.
 
    @param group_name   The group name.
    @param name         The name of the property -- must be unique in the group.
@@ -342,7 +342,7 @@ class GeometryProperties {
     }
   }
 
-  /** Retrieves that type-erased value for the property (`group_name`, `name`)
+  /** Retrieves the type-erased value for the property (`group_name`, `name`)
    from this set of properties.
 
    @param group_name  The name of the group to which the property belongs.
@@ -406,7 +406,7 @@ class GeometryProperties {
     return kDefaultGroup.access();
   }
 
-  /** Removes the ('group_name', 'name') property (if it exists). Upon
+  /** Removes the (`group_name`, `name`) property (if it exists). Upon
    completion the property will not be in the set.
    @returns `true` if the property existed prior to the call.  */
   bool RemoveProperty(const std::string& group_name, const std::string& name);
@@ -447,7 +447,7 @@ class GeometryProperties {
  private:
   // Conditionally writes the property (group_name, name) with the given value.
   // The caller provides a test function that should throw if assigning `value`
-  // to that property's group is an error. The function takes the `Group`
+  // to the specified property would fail. The function takes the `Group`
   // associated with `group_name`.
   void WritePropertyAbstract(
       const std::string& group_name, const std::string& name,

geometry/geometry_state.cc

@@ -739,6 +739,12 @@ template <typename T>
 void GeometryState<T>::AssignRole(SourceId source_id, GeometryId geometry_id,
                                   PerceptionProperties properties,
                                   RoleAssign assign) {
+  if (assign == RoleAssign::kReplace) {
+    throw std::logic_error(
+        "AssignRole() with RoleAssign::kReplace does not work for perception "
+        "properties");
+  }
+
   InternalGeometry& geometry =
       ValidateRoleAssign(source_id, geometry_id, Role::kPerception, assign);
 
@@ -779,14 +785,26 @@ void GeometryState<T>::AssignRole(SourceId source_id, GeometryId geometry_id,
         "roles");
   }
 
+  // TODO(SeanCurtis-TRI): We want to remove this warning. For that to happen,
+  // we need systems dependent on illustration properties to recognize if there
+  // has been a change since last they processed the state. The simplest way to
+  // do that is to have a monotonically increasing serial number on
+  // GeometryState. It would increment for *every* change to GeometryState. A
+  // visualizer could query to determine if the serial number matches the value
+  // it had when it last initialized. If not, it can re-intialize (whatever that
+  // entails). It might also be advisable to have a collection of serial numbers
+  // with reduced scope (i.e., so a change to proximity properties doesn't
+  // cause illustration systems to reinitialize).
+  if (assign == RoleAssign::kReplace) {
+    static logging::Warn log_once(
+        "Updating illustration role properties must be done before visualizer "
+        "initialization to have an effect. When in doubt, after making "
+        "property changes, force the visualizer to re-initialize via its API.");
+  }
+
   InternalGeometry& geometry =
       ValidateRoleAssign(source_id, geometry_id, Role::kIllustration, assign);
-  // TODO(SeanCurtis-TRI): Ideally, if assign == RoleAssign::kReplace, this
-  //  should cause the visualization to change. I.e., if I've loaded an object
-  //  then I change its color here, it would be great if the visualization
-  //  reflected this. That is a *huge* issue that is not easily resolved.
-  //  Alternatively, I need to document that this *doesn't* happen and that it
-  //  is up to the visualizer to re-initialize itself.
+
   geometry.SetRole(std::move(properties));
 }
 
@@ -1184,12 +1202,6 @@ InternalGeometry& GeometryState<T>::ValidateRoleAssign(SourceId source_id,
                                                        GeometryId geometry_id,
                                                        Role role,
                                                        RoleAssign assign) {
-  if (assign == RoleAssign::kReplace &&
-      (role == Role::kPerception || role == Role::kIllustration)) {
-    throw std::logic_error(
-        "AssignRole() for updating properties currently only supports "
-        "proximity properties");
-  }
   if (!BelongsToSource(geometry_id, source_id)) {
     throw std::logic_error("Given geometry id " + to_string(geometry_id) +
         " does not belong to the given source id " +

geometry/internal_geometry.h

@@ -171,18 +171,15 @@ class InternalGeometry {
    engine's understanding as well.  */
   //@{
 
-  /** Assigns a proximity role to this geometry, replacing any properties that
-   were previously assigned.  */
+  /** Assigns a proximity role to this geometry, replacing any proximity
+   properties that were previously assigned.  */
   void SetRole(ProximityProperties properties) {
     proximity_props_ = std::move(properties);
   }
 
-  /** Assigns a illustration role to this geometry. Fails if it has already been
-   assigned.  */
+  /** Assigns an illustration role to this geometry, replacing any illustration
+   properties that were previously assigned.  */
   void SetRole(IllustrationProperties properties) {
-    if (illustration_props_) {
-      throw std::logic_error("Geometry already has illustration role assigned");
-    }
     illustration_props_ = std::move(properties);
   }
 

geometry/proximity/test/hydroelastic_internal_test.cc

@@ -683,9 +683,9 @@ void TestPropertyErrors(
     // This error message comes from GeometryProperties::GetProperty().
     DRAKE_EXPECT_THROWS_MESSAGE(
         maker(shape_spec, wrong_value), std::logic_error,
-        fmt::format(".*The property '{}' .+ exists, but is of a different "
-                    "type.+string'",
-                    property_name));
+        fmt::format(".*The property \\('{}', '{}'\\) exists, but is of a "
+                    "different type.+string'",
+                    group_name, property_name));
   }
 
   // Error case: property value is not positive.

geometry/scene_graph.cc

@@ -271,6 +271,11 @@ void SceneGraph<T>::AssignRole(Context<T>* context, SourceId source_id,
                                GeometryId geometry_id,
                                IllustrationProperties properties,
                                RoleAssign assign) const {
+  static const logging::Warn one_time(
+      "Due to a bug (see issue #13597), changing the illustration roles or "
+      "properties in the context will not have any apparent effect in, at "
+      "least, drake_visualizer. Please change the illustration role in the "
+      "model prior to allocating the Context.");
   auto& g_state = mutable_geometry_state(context);
   g_state.AssignRole(source_id, geometry_id, std::move(properties), assign);
 }

geometry/scene_graph.h

@@ -534,7 +534,7 @@ class SceneGraph final : public systems::LeafSystem<T> {
 
    <h4>Changing the properties for an assigned role</h4>
 
-   If  the geometry has previously been assigned a role, the properties for
+   If the geometry has previously been assigned a role, the properties for
    that role can be modified with the following code (using ProximityProperties
    as an example):
 
@@ -560,13 +560,7 @@ class SceneGraph final : public systems::LeafSystem<T> {
    // Remove a property previously assigned.
    new_props.RemoveProperty("old_group", "old_name_1");
    // Update the *value* of an existing property (but enforce same type).
-   new_props.AddProperty("old_group", "old_name_2", new_value,
-                         AddPolicy::kUpdate);
-   // Overwrite the type and value of an existing property. This is the most
-   // dangerous modification; only do it if you're confident a type change is
-   // appropriate.
-   new_props.AddProperty("old_group", "type_A_prop", type_B_value,
-                         AddPolicy::kOverwrite);
+   new_props.UpdateProperty("old_group", "old_name_2", new_value);
    scene_graph.AssignRole(source_id, geometry_id, new_props,
                           RoleAssign::kReplace);
    @endcode
@@ -575,9 +569,11 @@ class SceneGraph final : public systems::LeafSystem<T> {
    role; it will simply eliminate possibly necessary properties. To remove
    the role completely, call `RemoveRole()`.
 
-   @warning Currently, only __proximity__ properties can be updated via this
-   mechanism. Updating illustration and perception will throw an exception (to
-   be implemented in the near future).
+   @warning Currently, only __proximity__ and __illustration__ properties can be
+   updated via this mechanism. Updating illustration properties has limitations
+   (see @ref AssignRole(SourceId,GeometryId,IllustrationProperties,RoleAssign)
+   "AssignRole(..., IllustrationProperties)" below). Attempting to update
+   perception will throw an exception (to be implemented in the near future).
 
    All invocations of `AssignRole()` will throw an exception if:
 
@@ -637,6 +633,13 @@ class SceneGraph final : public systems::LeafSystem<T> {
                   RoleAssign assign = RoleAssign::kNew) const;
 
   /** Assigns the illustration role to the geometry indicated by `geometry_id`.
+
+   @warning When changing illustration properties
+   (`assign = RoleAssign::kReplace`), there is no guarantee that these changes
+   will affect the visualization. The visualizer needs to be able to
+   "initialize" itself after changes to properties that will affect how a
+   geometry appears. If changing a geometry's illustration properties doesn't
+   seem to be affecting the visualization, retrigger its initialization action.
    @pydrake_mkdoc_identifier{illustration_direct}
    */
   void AssignRole(SourceId source_id, GeometryId geometry_id,
@@ -647,6 +650,19 @@ class SceneGraph final : public systems::LeafSystem<T> {
    @ref AssignRole(SourceId,GeometryId,IllustrationProperties) "AssignRole()"
    for illustration properties. Rather than modifying %SceneGraph's model, it
    modifies the copy of the model stored in the provided context.
+
+   @warning When changing illustration properties
+   (`assign = RoleAssign::kReplace`), there is no guarantee that these changes
+   will affect the visualization. The visualizer needs to be able to
+   "initialize" itself after changes to properties that will affect how a
+   geometry appears. If changing a geometry's illustration properties doesn't
+   seem to be affecting the visualization, retrigger its initialization action.
+
+   @warning Due to a bug (see issue
+   <a href="https://github.com/RobotLocomotion/drake/issues/13597">#13597</a>),
+   changing the illustration roles or properties in a systems::Context will not
+   have any apparent effect in, at least, drake_visualizer. Please change the
+   illustration role in the model prior to allocating the context.
    @pydrake_mkdoc_identifier{illustration_context}
    */
   void AssignRole(systems::Context<T>* context, SourceId source_id,

geometry/test/geometry_properties_test.cc

@@ -83,7 +83,7 @@ GTEST_TEST(GeometryProperties, UpdateProperty) {
   TestProperties properties;
 
   // Initialize with a single property.
-  const string& group_name{"some_group"};
+  const string group_name{"some_group"};
   const string prop_name("some_property");
   const int int_value{7};
   DRAKE_EXPECT_NO_THROW(
@@ -137,10 +137,10 @@ GTEST_TEST(GeometryProperties, RemoveProperty) {
 
   // Trying to remove it again has no effect.
   EXPECT_FALSE(properties.RemoveProperty(group1, prop1));
-  ASSERT_FALSE(properties.HasProperty(group1, prop1));
-  ASSERT_TRUE(properties.HasProperty(group1, prop2));
-  ASSERT_TRUE(properties.HasProperty(group2, prop1));
-  ASSERT_TRUE(properties.HasProperty(group2, prop2));
+  EXPECT_FALSE(properties.HasProperty(group1, prop1));
+  EXPECT_TRUE(properties.HasProperty(group1, prop2));
+  EXPECT_TRUE(properties.HasProperty(group2, prop1));
+  EXPECT_TRUE(properties.HasProperty(group2, prop2));
 }
 
 // Struct for the AddPropertyStruct test.