geometry: Figure out plan for public API for ShapeReifier

[EricCousineau-TRI]

UPDATE: See this comment for current plan of action.

UPDATE 2: Changed title to better capture my concerns (plans for ShapeReifier as public API).

---

Old:

Follow-up on Anzu PR, and following from some brainstorming with @calderpg-tri:

At present, the ShapeReifier passed void* for user data; this is very type unsafe, and promotes repetitive unsafe operations that could be messed up.

I'd propose one of the following options:

• Make user implementations store state in order to store data, kinda lambda-esque style.
  • It's already stateful 'cause it has a vtable.
• Use AbstractValue. If users need references, then just make it Value<T*> (not <T>), s.t. some level of type safety is guaranteed by RTTI.

My vote is for the first. Given that this is a new interface, I'd be fine with backwards incompatible changes.

My est. is that this is a 30min-1hr project, from scratch to first PR, with either implementation.

\cc @SeanCurtis-TRI @sherm1

[SeanCurtis-TRI]

I drag my feet on this because of the perpetually grinding rendering PR chain -- every time a change like this goes through, I have to rebase and revise. It leads to an increase in my programming overhead (work that doesn't actually produce anything new). That's why there are a number of technical debt issues that I've been deferring until I'm done. That said, I recognize it's not a strong reason to defer alleviating technical debt. If you feel strongly about this, then I'd say go ahead.

There are currently six derived ShapeReifier classes in Drake (plus whatever's in Anzu). The proximity engines (1 solution applied twice), the render engine (1 solution applied once), and the shape reifier test make use of the user_data and would have to be modified. The ShapeToLcm would not require modification (as it does not make use of user data).

[jwnimmer-tri]

As a middle ground, we could mark the user_data as deprecated and stop using it in new code -- with a TODO (or this issue) to go back and rip it out later?

[calderpg-tri]

I agree that void* is bad, but I don't really agree that just because a ShapeReifier has a vtable that the user should now have to use internal state when many operations do fit the current API. I think we've painted ourselves into a corner here where templating around the user data type would be nice, but that doesn't mix with virtual functions. I would prefer that we think more about a better reify interface that doesn't put us in this corner.

[SeanCurtis-TRI]

I missed the part where templates were brought into play.

If I had to vote between internal state and AbstractValue, I'd vote for AbstractValue. I considered state but really didn't like the very indirect connection between a value being used downstream and where it was set upstream. I felt passing it into reify made the connection more visceral. And since the AbstractValue is essentially a discplined void*, that seems the logical transition even though I haven't fully thought it through in terms of the user data being optional and what the details would look like.

On the other hand, if we went the "state route", the visualizers would be unaffected (as they don't really need to pass user data around). It would only impact the geometry internals (which arise from trying to reduce the FCL/VTK boilerplate.) So, I'm not wholly adverse to state-based.

Finally, to be perfectly frank, the reify infrastructure is difficult to grok in the first place -- the things we do to avoid RTTI, so I'd rather not make it any more opaque.

[sherm1]

Are you sure this is solving an actual problem? A rarely-used void* loophole for dealing with an outside library doesn't seem that terrible to me.

[EricCousineau-TRI]

For me, it's solving a problem that seems easy enough to fix with a minor pattern change, and something that I wouldn't have let through platform review (since it seems really easy to fix?).

I'm also thinking about Python (specifically, the meshcat visualizer): If this is to stay void*, then it gets slightly more nasty in that I have to deal with PyObject* junk, whereas AbstractValue would keep things simple + safe.

Also, for the Python story, the reifier also isn't the best interface, as double dispatch means nothing to Python. Going the state-based route, rather than a visitor-type pattern, would make things simpler as far as that story goes.

EDIT: As far as deprecation goes, it's only been there for a day, so I'd vote just change it now?

[EricCousineau-TRI]

As a compromise, I'll go ahead and submit a PR to expose some level of introspection capabilities to Python with the current void* workflow. If it's overly painful, I'll just work in the AbstractValue* workaround. If it ain't, I'll just stick with it as-is. (I may tweak the ShapeReifier interface in other ways, but will avoid touching user_data for now.)

[EricCousineau-TRI]

Followup: I had originally thought that the ShapeReifier interface was for a visitor-type patter, where it would visit each shape. I now realize that SceneGraphInspector::GetShape(...) exists, in which case, there is absolutely zero need for any reification in Python.

So I guess now my concerns are just (a) type safety and (b) backwards compatibility.

I see the use cases now where ShapeReifier is the base class for things like RenderEngine, where it makes sense (per this implementation) to provide user data. Given the amount of data needed for shape implementation (e.g. in RenderEngineVtk), it also makes sense that it be part of that class rather than trying to dispatch with different pointers.

Regarding (a) and (b), these only matter if the ShapeReifier is expected to be part of the public API (which I think we can discuss tomorrow).

If it is to be part of the public API:

• For (a), I would really like to minimize foot-guns for users, and use Value<T*>.
  • Perhaps making ShapeReifier be internal, and then providing a more user-friendly version that uses Value<T*>?
• For (b), it'd be nice to expose a version that doesn't create linker errors the moment you introduce a new shape type.
  • This can be solved by the same above point - making the public reifier have a default implementation that only throws at runtime.

Can discuss more f2f in meeting tomorrow.

[EricCousineau-TRI]

This seems to be moot now since the API has been working, and reification is accessible in Python through simpler means: https://github.com/ericcousineau-tri/repro/pull/1#pullrequestreview-402185013