set_property on nested properties

jwnimmer-tri commented 9 months ago

Sometimes, a user might want to change a property that is nested within the scene_tree's object.

For example:

{
  "type": "set_property",
  "path": "/Lights/PointLightNegativeX/<object>",
  "property": "shadow.radius",
  "value": 1.15
}

If the user attempts this today, Meshcat will do scene_node.object["shadow.radius"] = 1.15.

Instead, we want it to do scene_node.object["shadow"]["radius"] = 1.15.

jwnimmer-tri commented 9 months ago

Possibly https://github.com/meshcat-dev/meshcat/pull/137 is an implementation of this idea.

SeanCurtis-TRI commented 8 months ago

As I'm working on this and looking at the semantics, there are some details on which I'd like to solicit opinions.

Given a property path a.b.c and a value x, there's a few ways that the request to set_property(path, 'a.b.c'. x) can be ill formed. For nomenclature's sake, by the time we get to set_property we've resolved path to an object (which is properly a javascript Object). So, we're reasoning about a path like <object>a.b.c.

A named property simply doesn't exist. (<object> does exist, but a, b, or c could be undefined).
A named property exists exists but can't accept properties (e.g., a.b. refers to a double, rendering c nonsense).

In master, if we call set_property(path, 'a', x):

(1): The resolved property path would be <object>.a. <object> exists by definition. If a doesn't exist, we simply add a new property to <object> called a. Whether or not this has value, depends.
(2): This outcome isn't possible, because <object> always accepts properties. (See the previous point.)

However, things are different when we examine a longer chain (e.g., <object>a.b.c. What we do in the code will affect outcomes.

Options

Do nothing

We mindlessly traverse down an ostensible sequence of objects: <object>[a][b][c] = value. If either (1) or (2) applies to any of those internal names, we'll get a javascript exception ("TypeError: cannot set properties of [something]", where something may be undefined or some other data type that can't accept properties).

Allowing a user typo to shut down all of the meshcat session is not ideal.

Add where we can, catch where we can't

(1): If a name doesn't map to a property, we add the property with an Object type. By induction, this precludes the possibility of a missing intermediate property ever leading errors due to (2).
(2): The user can still name an existing property and reference an impossible child property (i.e., in <object>.a.b.c, a is type double.) We would have to detect this, print an error and, otherwise, make sure the call to set_property a no-op. Note: it won't literally be a no-op, because evaluating the path property may have created objects in the scene graph with their own extant properties; the act of looking for a path creates it. These wouldn't be backed out.

Essentially, we allow the user to create missing objects assuming that's the user's intention and these new objects will have value based on the user's further actions.

Warn where we can't, catch everywhere else.

(1): Rather than creating a missing intermediate object, we simply warn and treat the set_property() call as a no-op (with the same caveat as above).
(2): Same resolution as above.

The difference between those two options is whether we allow set_property() to have semantics to create properties/objects. We could also combine them. Warn that the path was missing, but we've created it, allowing for both.

While I'm generally inclined to the last (combination of both, create-and-warn where we can, skip where we can't), I'd like to see if anyone thinks I'm over-thinking/engineering this.

If there's no response, we'll see my PR manifesting my current preference in a bit.

jwnimmer-tri commented 8 months ago

Allowing a user typo to shut down all of the meshcat session is not ideal.

I don't follow this part. When the message has an error like this, we get an exception in the JS console but nothing stops working, does it? Can you give me a repro for a case were something "shuts down"?

jwnimmer-tri commented 8 months ago

Ah. Maybe you are thinking of cases of static HTML files instead of websockets? We might need to (and probably should anyway) wrap the HTML message replay code to log exceptions and keep going.

SeanCurtis-TRI commented 8 months ago

No, you're right about exceptions not hanging the browser or the 3D scene. I was deceived by the browsers behavior change when the console is open.

Nevertheless, #177 is ready for review.

SeanCurtis-TRI commented 8 months ago

But yes....I hadn't thought explicitly about the static html from Drake. But it definitely applies to the static html in the test html in this repo.

jwnimmer-tri commented 8 months ago

If a name doesn't map to a property, we add the property with an Object type.

I understand the logic in this, but what is the use case?

Is there any situation in threejs where a property is absent and adding it changes what we see on the screen, or how something behaves? If not, I'm inclined to think "make a new property out of thin air" is likely to cause more harm and confusion that good -- I would rather identify some use case first, before we lock in that approach.

SeanCurtis-TRI commented 8 months ago

re: adding missing properties

It was born solely of the dynamic nature of javascript. I was imagining a case where someone might set a new property on something in the meshcat scene graph that would be used by their visualization webpage. I imagine someone putting a property in place that they would then use.

My initial version treated that simply as an error. I'm happy to go either way.

jwnimmer-tri commented 8 months ago

Yeah, if the only use is by out-of-band javascript, then I'm happy to let it go for now. They might as well write their own set_property hooks at that point, since they are already tweaking with the page.

I hadn't noticed that the PR took the always-error option. I must have read over it too quickly. _Edit: Ah, I think you meant your first local version did it that way. I think the PR adds the object_.

SeanCurtis-TRI commented 8 months ago

You didn't miss anything in the. The first draft of the branch was always error. By the time the PR opened, I went with the permissive approach.

So, we'll go for the strict approach. (Although, I can imagine a use case where I'd want to use geometry properties to pass onto meshcat so my page, written once, can make use of the model I've built dynamically in my simulator process.)

meshcat-dev / meshcat