RgbdRendererOspray::ImplRegisterVisual() -- only allows for a single geometry

RobotLocomotion / drake

Model-based design and verification for robotics.

https://drake.mit.edu

Other

3.35k stars 1.27k forks source link

RgbdRendererOspray::ImplRegisterVisual() -- only allows for a single geometry #9274

Closed SeanCurtis-TRI closed 4 years ago

SeanCurtis-TRI commented 6 years ago

Description

If multiple geometries are assigned to an RgbdRendererOspray only the last geometry will appear.

Solution

RgbdRendererOspray::Impl has two members:

std::array<vtkNew<vtkActor>, kNumOutputImage> actors_;
std::array<vtkNew<vtkPolyDataMapper>, kNumOutputImage> mappers_; For each registered visual, it assigns geometry to the common actors array and connects them to the common mappers.

If you contrast that with the implementation in RgbdRendererVTK::Impl you'll note that each invocation of ImplRegisterVisual() instantiates a unique set of actors and mappers.

RgbdRendererOspray should mirror this behavior. When it is modified to match, then all registered geometries appear.

jamiesnape commented 6 years ago

If I recall correctly, there are non-deterministic segfaults if the members in RgbdRendererOspray::Impl are changed to locals (https://github.com/RobotLocomotion/drake/pull/8939#pullrequestreview-135115320, FYI @EricCousineau-TRI)

EricCousineau-TRI commented 6 years ago

Yes, that is correct.

If you look at the diff between r11 and r12 on this file: https://reviewable.io/reviews/robotlocomotion/drake/8939/systems/sensors/rgbd_renderer_ospray.cc you'll note the shift from using locals to using member values.

Is there a way to resolve this without locals?

Or rather, can you run the unittests for this about 100-200x (in a shell for-loop or whatever) on an Ubuntu 16.04 Puget, and ensure that there are no segfaults in every single run?

SeanCurtis-TRI commented 6 years ago

I'm running it with locals and haven't had a single geometry-based segfault. In talking to @kunimatsu-tri the current working hypothesis is that it may be an artifact of its interaction with the test harness.

kunimatsu-tri commented 6 years ago

When I run VTK-OSPRay program with valgrind, regardless it's one in drake or stand alone cmake program, I get tons of valgrind errors.

Most of them are:

Conditional jump or move depends on uninitialised value(s)
Use of uninitialised value of size 8 from libospray_module_ispc.so.1.5.0.

And some of them are:

Invalid read of size 4 at destruction time and related to embree.so.2.17.4 and ospray::api::ISPCDevice.

I'm digging more and will report if I find something related.

kunimatsu-tri commented 6 years ago

I reverted what @EricCousineau-TRI pointed out and try running gtest.

One observation I think related so far is that if I leave only MeshTest in rgbd_renderer_ospray_test.cc and remove all the other tests, it runs no problem. I repeatedly ran 100 times for that test, and get zero unexpected termination which I had before by running all the test.

If I add one more test, say CylinderTest, back to rgbd_renderer_sopray_test.cc, and repeatedly run 100 times for that test, I get unexpected termination.

I still don't know why this happens.

EricCousineau-TRI commented 5 years ago

Placed the priority at medium given the activity surrounding this; lemme know if we want to change it.

jamiesnape commented 5 years ago

This is really coupled with #8978, so maybe this should be assigned to me also?

EricCousineau-TRI commented 5 years ago

Sounds good to me! (I'm sure @kunimatsu-tri won't complain :P)

SeanCurtis-TRI commented 5 years ago

I agree. It looks like this reported issue is the product of the proposed solution to the issues encountered in the original PR. Putting these issues into a single basket makes sense to me.

EricCousineau-TRI commented 5 years ago

(FTR Resolution is coupled to #8978 b/c we want to de-singleton these bits.)

SeanCurtis-TRI commented 4 years ago

Closing this as an attic artifact that no longer matters.