[geometry] IllustrationProperties can be updated after parsing

[SeanCurtis-TRI]

This PR has two curated commits:

1. The first extends GeometryProperties functionality to make it easier to modify properties. A mutable instance of GeometryProperties can have properties removed, updated, or overwritten.
2. The second enables AssignRole(..., IllustrationProperties, AssignRole::kReplace). Previously, that would throw. Due to issues in the greater ecosystem, this added functionality comes with limitations and warnings.

• there are warnings in the documentation.
• Warnings will likewise get printed to the console.

resolves #13126

(While not the total solution, it is sufficient to empower workflows to change visual appearance of parsed geometry.)

---

This change is 

[SeanCurtis-TRI]

cc @mposa I won't saddle you with feature review (unless you're game). But I did want you to see this so you could confirm that it will satisfy your needs.

+@DamrongGuoy for feature review.

Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

[DamrongGuoy]

Checkpoint. I'm still early in the review.

Reviewed 2 of 7 files at r1.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy and @SeanCurtis-TRI)

---

geometry/geometry_properties.h, line 59 at r2 (raw file):

 the AddProperty() method to add new properties to the set.
 <!-- TODO(SeanCurtis-TRI): As need becomes apparent, add interface to replace
 values in previously defined values -- while preserving the original type.

Does this PR solve this TODO? If so, please remove the TODO.

---

geometry/geometry_properties.cc, line 45 at r2 (raw file):

      throw std::logic_error(
          fmt::format("AddProperty(): Trying to add property ('{}', '{}'); "
                      "a property with that name already exists",

Nit: should this message be parallel to the next one by adding kUnique into the message? Perhaps something like this?

fmt::format("AddProperty(): Trying to add property ('{}', '{}') with kUnique  "
 "add policy. The property with that name exists already",

---

geometry/geometry_properties.cc, line 47 at r2 (raw file):

                      "a property with that name already exists",
                      group_name, name));
    } else if (add_policy == AddPolicy::kUpdate &&

BTW would you consider switch over if-else ? Perhaps something like this?

if (value_iter != group.end()) {
  switch (add_policy) {
    case AddPolicy::kUnique:
      throw...
    case AddPolicy::kUpdate:
      if (...) {
        throw...
      }
    default:    
  }
}

---

geometry/geometry_properties.cc, line 79 at r2 (raw file):

bool GeometryProperties::RemoveProperty(const std::string& group_name,
                                        const std::string& name) {
  auto iter = values_.find(group_name);

Nit: const auto iter ?

[SeanCurtis-TRI]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy and @SeanCurtis-TRI)

---

geometry/geometry_properties.cc, line 79 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit: const auto iter ?

OK

Not in this case. It must be a non-const iter so I get a non-const Group, so I can remove things from it.

[DamrongGuoy]

Checkpoint. I have reviewed the first commit r1 (GeometryProperties). I am reviewing the second commit r2 (AssignRole(IllustrationProperties, kReplace)).

Reviewed 5 of 7 files at r1, 2 of 6 files at r2.
Reviewable status: 11 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy and @SeanCurtis-TRI)

---

bindings/pydrake/test/geometry_test.py, line 398 at r1 (raw file):

        self.assertFalse(prop.HasProperty(group_name=default_group,
                                          name="test"))
        # Overwrite the property.

For completeness, should we test kUnique and kUpdate too? Or can we write a comment to explain why testing only kOverwrite is sufficient?

---

geometry/geometry_properties.cc, line 79 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

OK

Not in this case. It must be a non-const iter so I get a non-const Group, so I can remove things from it.

That is fine. No problem. I set to Satisfied.

Previously I thought we wanted:

const std::unordered_map<std::string, Group>::iterator iter = values_.find(group_name);

The iterator itself is const, but it points to a non-const entry.

Now I'm not sure whether what I suggested const auto iter = values_.find(group_name) would give:

const std::unordered_map<std::string, Group>::iterator

or

const std::unordered_map<std::string, Group>::const_iterator

---

geometry/geometry_state.cc, line 792 at r2 (raw file):

  // that a visualizer could query to determine if its view of the world is
  // stale would be enough. It could then opt to re-initialize based on the new
  // state.

I do not completely understand this TODO and the next TODO 10 lines below. Are they redundant, complementary, or coupled?

I'm fine with keeping them both. I just double check here.

---

geometry/scene_graph.h, line 566 at r1 (raw file):

                         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

BTW, would "most advanced modification" sound better than "most dangerous modification" ?

---

geometry/test/geometry_properties_test.cc, line 67 at r2 (raw file):

  // Confirm existence.
  ASSERT_TRUE(properties.HasProperty(group_name, prop_name));
  int read_value = properties.GetProperty<int>(group_name, prop_name);

Nit: const int read_value ?

---

geometry/test/geometry_properties_test.cc, line 90 at r2 (raw file):

  EXPECT_NO_THROW(properties.AddProperty(group_name, prop_name, int_value + 1,
                                         AddPolicy::kUpdate));
  EXPECT_EQ(properties.GetProperty<int>(group_name, prop_name), int_value + 1);

Please add the test for kUpdate for "already exists and is of different type". Perhaps something like this?

// Update with different type is invalid; we throw.
DRAKE_EXPECT_THROWS_MESSAGE(
      properties.AddProperty(group_name, prop_name, double_value,
                             AddPolicy::kUpdate),
      std::logic_error,
      ".*Trying to add property \\('.+', '.+'\\) with kUpdate.+"
      "property already exists and is of different type. .+");
EXPECT_EQ(properties.GetProperty<int>(group_name, prop_name), int_value + 1);

---

geometry/test/geometry_properties_test.cc, line 99 at r2 (raw file):

  // Test kOverwrite add policy.
  const double double_value = 17.3;
  EXPECT_NO_THROW(properties.AddProperty(group_name, prop_name, double_value,

Please add a comment like this to help readers and to be consistent with other tests.

// Update with different type is valid; we change the type.

---

geometry/test/geometry_properties_test.cc, line 103 at r2 (raw file):

  EXPECT_EQ(properties.GetProperty<double>(group_name, prop_name),
            double_value);
}

Please add the test for kOverwrite when there was no pre-existing property. The documentation of kOverwrite is:

kOverwrite  ///< The property will be set, regardless of whether there was a
            ///< pre-existing property.

We have tested when there was a pre-existing property already. We should add a test when there was no pre-existing property.

Perhaps the test could be like this?

  // AddProperty with kOverwrite for new property is valid.
  const std::string alt_prop = prop_name + "alt";
  EXPECT_NO_THROW(properties.AddProperty(group_name, alt_prop, int_value,
                                         AddPolicy::kOverwrite));
  EXPECT_EQ(properties.GetProperty<int>(group_name, alt_prop), int_value);

---

geometry/test/internal_geometry_test.cc, line 58 at r2 (raw file):

    EXPECT_FALSE(
        geometry.illustration_properties()->HasProperty("props2", "value"));
    // Resetting proximity properties is not an error.

Nit: typo. s/proximity/illustration.

[DamrongGuoy]

I have finished the review.

Reviewed 4 of 6 files at r2.
Reviewable status: 12 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

---

geometry/test/geometry_state_test.cc, line 2534 at r2 (raw file):

  SetUpSingleSourceTree();

  // Case: confirm throw for perception and illustration properties.

Nit: s/perception and illustration/perception.

[SeanCurtis-TRI]

Thanks for the thorough review. Your comments have led to some changes. PTAL.

Specifically, AddPolicy is gone and, in its place, is just UpdateProperties. The "overwrite" functionality is now gone.

Reviewable status: 4 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

---

bindings/pydrake/test/geometry_test.py, line 398 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

For completeness, should we test kUnique and kUpdate too? Or can we write a comment to explain why testing only kOverwrite is sufficient?

Tests have changed to reflect change in API.

---

geometry/geometry_properties.h, line 59 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Does this PR solve this TODO? If so, please remove the TODO.

Done

Great eye. I hadn't noticed that.

---

geometry/geometry_properties.cc, line 45 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit: should this message be parallel to the next one by adding kUnique into the message? Perhaps something like this?

fmt::format("AddProperty(): Trying to add property ('{}', '{}') with kUnique  "
 "add policy. The property with that name exists already",

Changed with API change.

---

geometry/geometry_properties.cc, line 47 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW would you consider switch over if-else ? Perhaps something like this?

if (value_iter != group.end()) {
  switch (add_policy) {
    case AddPolicy::kUnique:
      throw...
    case AddPolicy::kUpdate:
      if (...) {
        throw...
      }
    default:    
  }
}

No longer relevant.

---

geometry/geometry_properties.cc, line 79 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

That is fine. No problem. I set to Satisfied.

Previously I thought we wanted:

const std::unordered_map<std::string, Group>::iterator iter = values_.find(group_name);

The iterator itself is const, but it points to a non-const entry.

Now I'm not sure whether what I suggested const auto iter = values_.find(group_name) would give:

const std::unordered_map<std::string, Group>::iterator

or

const std::unordered_map<std::string, Group>::const_iterator

Oops. My bad. You are right. It's a const iterator not a const_iterator.

---

geometry/geometry_state.cc, line 792 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

I do not completely understand this TODO and the next TODO 10 lines below. Are they redundant, complementary, or coupled?

I'm fine with keeping them both. I just double check here.

First rephrased, second removed.

---

geometry/scene_graph.h, line 566 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, would "most advanced modification" sound better than "most dangerous modification" ?

Your comment forced me to reconsider even having the option. I definitely meant the word "dangerous". But, in retrospect, there's no proven need for it. It exists merely because it's the logical extrapolation of the first two cases. So, it doesn't make sense to facilitate a dangerous activity.

So, I'm changing the design here. There's now only three operations: Add, Update, and Remove.

---

geometry/test/geometry_properties_test.cc, line 67 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit: const int read_value ?

Done

---

geometry/test/geometry_properties_test.cc, line 90 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Please add the test for kUpdate for "already exists and is of different type". Perhaps something like this?

// Update with different type is invalid; we throw.
DRAKE_EXPECT_THROWS_MESSAGE(
      properties.AddProperty(group_name, prop_name, double_value,
                             AddPolicy::kUpdate),
      std::logic_error,
      ".*Trying to add property \\('.+', '.+'\\) with kUpdate.+"
      "property already exists and is of different type. .+");
EXPECT_EQ(properties.GetProperty<int>(group_name, prop_name), int_value + 1);

Done

---

geometry/test/geometry_properties_test.cc, line 99 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Please add a comment like this to help readers and to be consistent with other tests.

// Update with different type is valid; we change the type.

Done

---

geometry/test/geometry_properties_test.cc, line 103 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Please add the test for kOverwrite when there was no pre-existing property. The documentation of kOverwrite is:

kOverwrite  ///< The property will be set, regardless of whether there was a
            ///< pre-existing property.

We have tested when there was a pre-existing property already. We should add a test when there was no pre-existing property.

Perhaps the test could be like this?

  // AddProperty with kOverwrite for new property is valid.
  const std::string alt_prop = prop_name + "alt";
  EXPECT_NO_THROW(properties.AddProperty(group_name, alt_prop, int_value,
                                         AddPolicy::kOverwrite));
  EXPECT_EQ(properties.GetProperty<int>(group_name, alt_prop), int_value);

OK
No longer relevant

---

geometry/test/geometry_state_test.cc, line 2534 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit: s/perception and illustration/perception.

Done

---

geometry/test/internal_geometry_test.cc, line 58 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit: typo. s/proximity/illustration.

Done

[DamrongGuoy]

I almost finish reviewing the newer simpler design. I'll review geometry_properties_test.cc later. That's the only thing left.

Reviewed 8 of 9 files at r3.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy and @SeanCurtis-TRI)

---

geometry/geometry_properties.h, line 223 at r3 (raw file):

 }

*/

This class document already has <h3>Reading properties</h3>, should it have <h3>Updating and removing properties</h3> too ?

---

geometry/geometry_properties.h, line 269 at r3 (raw file):

  /** 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

Nit:s/"its value the same type"/"its value has the same type"

---

geometry/geometry_properties.h, line 284 at r3 (raw file):

  }

  /** Adds the named property (`group_name`, `name`) with the giventype-erased

Nit: s/giventype-erased/given type-erased (space between given and type)

---

geometry/geometry_properties.h, line 345 at r3 (raw file):

  }

  /** Retrieves that type-erased value for the property (`group_name`, `name`)

Nit:s/"Retrieves that type-erased value"/"Retrieves the type-erased value"

---

geometry/geometry_properties.h, line 409 at r3 (raw file):

  }

  /** Removes the ('group_name', 'name') property (if it exists). Upon

Nit: switch ticks and backticks: 'group_name', 'name' => `group_name`, `name`

---

geometry/geometry_properties.cc, line 55 at r3 (raw file):

            group.at(name)->type_info() != value.type_info()) {
          throw std::logic_error(
              fmt::format("AddProperty(): Trying to update property ('{}', '{}'"

Please change the error message from "AddProperty()" to "UpdateProperty()".

---

geometry/geometry_properties.cc, line 59 at r3 (raw file):

                          "type. New type {}, existing type {}",
                          group_name, name, group.at(name)->GetNiceTypeName(),
                          value.GetNiceTypeName()));

The message is New type {} followed by existing type {}, but the arguments are in the opposite order: group.at(name)->GetNiceTypeName() followed by value.GetNiceTypeName().

Please make the order consistent.

---

geometry/scene_graph.h, line 569 at r3 (raw file):

   // appropriate.
   new_props.AddProperty("old_group", "type_A_prop", type_B_value,
                         AddPolicy::kOverwrite);

Please update the example in the documentation to remove:

   new_props.AddProperty("old_group", "old_name_2", new_value,
                         AddPolicy::kUpdate);
   new_props.AddProperty("old_group", "type_A_prop", type_B_value,
                         AddPolicy::kOverwrite);

and add:

   new_props.UpdateProperty("old_group", "old_name_2", new_value);

---

geometry/proximity/test/hydroelastic_internal_test.cc, line 683 at r3 (raw file):

    ProximityProperties wrong_value(props);
    wrong_value.AddProperty(group_name, property_name, "10");
    // This error message comes from GeometryProperties::GetProperty().

FYI, I had to jump around this call tree to look for the error message:

GeometryProperties::GetProperty()
-> GetPropertyAbstract(group_name, name);
        throw std::logic_error(fmt::format(
        "GetProperty(): There is no property ('{}', '{}')", group_name, name));
-> GetValueOrThrow<ValueType>("GetProperty", group_name, name,
                                        abstract);
          throw std::logic_error(fmt::format(
          "{}(): The property ('{}', '{}') exists, but is of a different type. "
          "Requested '{}', but found '{}'",
          method, group_name, name, nice_type_name,
          abstract.GetNiceTypeName()));

The error message is in GeometryProperties::GetValueOrThrow(), which is called by GetProperty().

No need to change anything.

[DamrongGuoy]

I've finished the review. with some requests.

Reviewed 1 of 9 files at r3.
Reviewable status: 9 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

[SeanCurtis-TRI]

Thanks @DamrongGuoy

+@sherm1 for platform review, please.

Reviewable status: 4 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy and @sherm1)

---

geometry/geometry_properties.h, line 223 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

This class document already has <h3>Reading properties</h3>, should it have <h3>Updating and removing properties</h3> too ?

I'd considered that. But that section is more about workflow and not strictly mechanics. I couldn't come up with a similar discussion about updating or removing that paralleled the workflow theme, so I left it out.

---

geometry/geometry_properties.h, line 269 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit:s/"its value the same type"/"its value has the same type"

Done

---

geometry/geometry_properties.h, line 284 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit: s/giventype-erased/given type-erased (space between given and type)

Done

---

geometry/geometry_properties.h, line 345 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit:s/"Retrieves that type-erased value"/"Retrieves the type-erased value"

Done

---

geometry/geometry_properties.h, line 409 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Nit: switch ticks and backticks: 'group_name', 'name' => `group_name`, `name`

Done

(Good eye!)

---

geometry/geometry_properties.cc, line 55 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Please change the error message from "AddProperty()" to "UpdateProperty()".

Done

---

geometry/geometry_properties.cc, line 59 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

The message is New type {} followed by existing type {}, but the arguments are in the opposite order: group.at(name)->GetNiceTypeName() followed by value.GetNiceTypeName().

Please make the order consistent.

Done

---

geometry/scene_graph.h, line 569 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Please update the example in the documentation to remove:

   new_props.AddProperty("old_group", "old_name_2", new_value,
                         AddPolicy::kUpdate);
   new_props.AddProperty("old_group", "type_A_prop", type_B_value,
                         AddPolicy::kOverwrite);

and add:

   new_props.UpdateProperty("old_group", "old_name_2", new_value);

Done

[DamrongGuoy]

Reviewed 3 of 3 files at r4.
Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)

---

geometry/geometry_properties.h, line 223 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'd considered that. But that section is more about workflow and not strictly mechanics. I couldn't come up with a similar discussion about updating or removing that paralleled the workflow theme, so I left it out.

Thank you for explanation.

[sherm1]

Platform with a few minor comments, ptal.

Reviewed 1 of 7 files at r1, 2 of 6 files at r2, 7 of 9 files at r3, 3 of 3 files at r4.
Reviewable status: 14 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

---

geometry/geometry_properties.h, line 270 at r4 (raw file):

   If the property doesn't already exist, it is equivalent to calling
   `AddProperty`. If the property does exist, and its value has the same type as
   `value`, the property value will be updated.

minor: consider "If the property does exist, its value (which must have the same type as value) will be replaced."

---

geometry/geometry_properties.h, line 297 at r4 (raw file):

   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.

minor: consider "If the property does exist, its value (which must have the same type as value) will be replaced."

---

geometry/geometry_properties.h, line 450 at r4 (raw file):

  // 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`

minor: consider rewording (sounds like it is only about value type mismatch and is confusing since a property group doesn't have a value). Something like:
"... that should throw if assigning value to the specified property would fail. Different operations have different requirements, for example AddProperty requires that the property does not already exist while UpdateProperty works either way but restricts the value type."

---

geometry/geometry_state.cc, line 745 at r4 (raw file):

    throw std::logic_error(
        "AssignRole() with RoleAssign::kReplace does not work for perception "
        "properties");

nit: missing period

---

geometry/geometry_state.cc, line 797 at r4 (raw file):

  // 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).

btw possible alternative?

• visualizer registers a callback with SG that gets invoked upon GeoState change, perhaps with flag saying what changed (could send an LCM message, e.g.)

---

geometry/geometry_state.cc, line 801 at r4 (raw file):

    static logging::Warn log_once(
        "Updating illustration role properties must be done before visualizer "
        "initialization to have an affect. When in doubt, after making "

nit affect -> effect (n.)

---

geometry/internal_geometry.h, line 180 at r4 (raw file):

  }

  /** Assigns a illustration role to this geometry, replacing any properties that

btw line looks too long in Reviewable -- might be ok though, please check

---

geometry/internal_geometry.h, line 180 at r4 (raw file):

  }

  /** Assigns a illustration role to this geometry, replacing any properties that

nit "a illustration" -> "an illustration"

---

geometry/internal_geometry.h, line 181 at r4 (raw file):

  /** Assigns a illustration role to this geometry, replacing any properties that
   were previously assigned.  */

minor "... replacing any illustration properties that were ..." (I think)

---

geometry/scene_graph.h, line 665 at r4 (raw file):

   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.

nit consider either Context or `context`to clarify whether you mean the class or the particular parameter here. I think Context is slightly better (and you can match it in the error message).

---

geometry/scene_graph.cc, line 278 at r4 (raw file):

      "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");

btw consider capitalizing Context here?

---

geometry/test/geometry_properties_test.cc, line 86 at r4 (raw file):

  // Initialize with a single property.
  const string& group_name{"some_group"};

minor: should be just string rather than string&

---

geometry/test/geometry_properties_test.cc, line 143 at r4 (raw file):

  ASSERT_TRUE(properties.HasProperty(group1, prop2));
  ASSERT_TRUE(properties.HasProperty(group2, prop1));
  ASSERT_TRUE(properties.HasProperty(group2, prop2));

nit all the above ASSERTs could be EXPECTs I think?

---

geometry/test/geometry_state_test.cc, line 2501 at r4 (raw file):

  props1.AddProperty("prop1", "value", 1);
  IllustrationProperties props2;
  props2.AddProperty("prop2", "value", 2);

minor: the names "prop1" and "prop2" confused me -- aren't these actually groups? And then the property names are both "value"? If so should give them group-y names like "group1" and "group2".

---

geometry/test/geometry_state_test.cc, line 2504 at r4 (raw file):

  // Generally, we assume that illustration property replacement exercises the
  // same infrastructure as perception

nit missing period. Also, not clear what the implication of this statement is. Can you elaborate?

---

geometry/test/geometry_state_test.cc, line 2529 at r4 (raw file):

}

// Test that attemptint to reassign perception properties to a geometry that

nit attemptint -> attempting

---

geometry/test/internal_geometry_test.cc, line 16 at r4 (raw file):

// Create two instances of the given Properties type with different properties:
// ('props1', 'value') in the first, ('props2', 'value') in the second.

minor: same comment -- isn't that ('group1', 'prop1') etc?

[SeanCurtis-TRI]

Comments addressed (and curated commits cleaned up).

Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy and @sherm1)

---

geometry/geometry_properties.h, line 270 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: consider "If the property does exist, its value (which must have the same type as value) will be replaced."

Done

---

geometry/geometry_properties.h, line 297 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: consider "If the property does exist, its value (which must have the same type as value) will be replaced."

Done

---

geometry/geometry_properties.h, line 450 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: consider rewording (sounds like it is only about value type mismatch and is confusing since a property group doesn't have a value). Something like:
"... that should throw if assigning value to the specified property would fail. Different operations have different requirements, for example AddProperty requires that the property does not already exist while UpdateProperty works either way but restricts the value type."

Done

---

geometry/geometry_state.cc, line 745 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: missing period

Drake is wildly inconsistent about punctuation on exception messages. Sometimes yes, sometimes no. I've been actively eschewing them for exception messages. No real reason, really. At this point, it's just habit.

---

geometry/geometry_state.cc, line 797 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

btw possible alternative?

• visualizer registers a callback with SG that gets invoked upon GeoState change, perhaps with flag saying what changed (could send an LCM message, e.g.)

Nah. You want to mix push semantics into my pull semantics? To say nothing of how that would work in transmogrification. I'll pass.

---

geometry/geometry_state.cc, line 801 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit affect -> effect (n.)

Done

---

geometry/internal_geometry.h, line 180 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

btw line looks too long in Reviewable -- might be ok though, please check

Done

---

geometry/internal_geometry.h, line 180 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit "a illustration" -> "an illustration"

Done

---

geometry/internal_geometry.h, line 181 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor "... replacing any illustration properties that were ..." (I think)

Done

This sequence of nits brought to you by copy-pasta!

---

geometry/scene_graph.h, line 665 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit consider either Context or `context`to clarify whether you mean the class or the particular parameter here. I think Context is slightly better (and you can match it in the error message).

Done

---

geometry/scene_graph.cc, line 278 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

btw consider capitalizing Context here?

Done

---

geometry/test/geometry_properties_test.cc, line 86 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: should be just string rather than string&

Done

---

geometry/test/geometry_properties_test.cc, line 143 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit all the above ASSERTs could be EXPECTs I think?

Done

---

geometry/test/geometry_state_test.cc, line 2501 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: the names "prop1" and "prop2" confused me -- aren't these actually groups? And then the property names are both "value"? If so should give them group-y names like "group1" and "group2".

Done

---

geometry/test/geometry_state_test.cc, line 2504 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit missing period. Also, not clear what the implication of this statement is. Can you elaborate?

Done

---

geometry/test/geometry_state_test.cc, line 2529 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit attemptint -> attempting

Done

---

geometry/test/internal_geometry_test.cc, line 16 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: same comment -- isn't that ('group1', 'prop1') etc?

Done

[sherm1]

Reviewed 8 of 8 files at r5.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

[jwnimmer-tri]

@SeanCurtis-TRI while it is not a defect (yet?) per our merge process, I'd like to encourage you when using "properly curated" style to frequently rebase the PR onto recent master, so that we don't have long stretches of parallel history like so (the brown line):

[SeanCurtis-TRI]

Thank you. I was uneasy when I hit merge, suspicious I'd forgotten something. And that was it. :-(

[jwnimmer-tri]

=> Reviewable/Reviewable#763 would allow us to automate this reminder

[SeanCurtis-TRI]

I like that.