dartsim / dart

DART: Dynamic Animation and Robotics Toolkit
http://dartsim.github.io/
BSD 2-Clause "Simplified" License
893 stars 286 forks source link

Collision-only shapes drawn by gui::osg #676

Closed costashatz closed 7 years ago

costashatz commented 8 years ago

Hello,

I observed weird colors when loading URDFs. I played a little bit with visuals/collisions and I came to the conclusion that the collision shapes are being drawn, as can be seen in the following picture (the light blueish links):

pexod

Is this really it? If yes, how can I disable it? Any other ideas?

Thanks,

mxgrey commented 8 years ago

Which branch are you using? I could imagine this either being a urdf parsing error or a rendering error. Perhaps a VisualAspect is being given to ShapeNodes that are only meant for collisions.

costashatz commented 8 years ago

I am using the master (upstream) branch..

mxgrey commented 8 years ago

Indeed, it looks like the OpenGL renderer may be rendering all ShapeNodes, whether or not they have been assigned a VisualAspect. It should be a pretty easy fix if this is the case. I'll patch this now and let you know when it's ready.

costashatz commented 8 years ago

I am using OSG by the way...

costashatz commented 8 years ago

Thanks!

mxgrey commented 8 years ago

The issue seems to be deeper and more convoluted than I anticipated, so it might take some time to work out a good solution. In the mean time, an easy way to dodge this issue would be to put this code right after you load the Skeleton:

// skel is a SkeletonPtr
for(size_t i=0; i < skel->getNumShapeNodes(); ++i)
{
  ShapeNode* node = skel->getShapeNode(i);
  if(!node->has<VisualAddon>())
  {
    node->create<VisualAddon>();
    node->getVisualAddon()->hide();
  }
}

The idea is that we look for any ShapeNode which is not supposed to have a VisualAddon. We then create a VisualAddon for it, but then immediately hide it. Luckily OSG should know how to hide them.

Note that this code will not work once #659 is merged because of API changes. Instead, you would want something more like this code:

// skel is a SkeletonPtr
for(size_t i=0; i < skel->getNumShapeNodes(); ++i)
{
  ShapeNode* node = skel->getShapeNode(i);
  if(!node->has<VisualAspect>())
  {
    node->createAspect<VisualAspect>();
    node->getVisualAspect()->hide();
  }
}
costashatz commented 8 years ago

Many thanks for the ultra fast response... I will test the quick hack and let you know if it works. What is more, I would be interested to know where is the problem exactly.. Thanks again..

mxgrey commented 8 years ago

The problem is primarily in dart/gui/osg/render. We use the ShapeFrame class to hold both visual geometry and collision geometry, and each ShapeFrame instance can have a VisualAddon, CollisionAddon, and/or DynamicsAddon (which in #659 get renamed to VisualAspect, CollisionAspect, and DynamicsAspect).

The renderers are supposed to assume that any ShapeFrame which lacks a VisualAspect should not get rendered, but in the current implementation they are instead creating a default-constructed VisualAspect for any ShapeFrame which does not have one. I attempted to fix this by short-circuiting this behavior so that the renderer will skip over any ShapeFrame without a VisualAspect instead of creating a VisualAspect for it, but this led to segmentation faults elsewhere.

Truthfully, the current OSG implementation is very messy. It was already messy back when I originally created it in 5.0, and then it became even messier when we quickly updated it to be compatible with the 6.0-beta. I don't have time for it right now, but I'd like to rethink the design of our OSG classes to make them more maintainable.

costashatz commented 8 years ago

Truthfully, the current OSG implementation is very messy. It was already messy back when I originally created it in 5.0, and then it became even messier when we quickly updated it to be compatible with the 6.0-beta. I don't have time for it right now, but I'd like to rethink the design of our OSG classes to make them more maintainable.

OK. Maybe I will spend some time over the next weeks to make something similar to what you have for OpenGL (GLUT). If you have anything let me know and I will do too..

mxgrey commented 8 years ago

I think the OpenGL implementation is even worse off than OSG at the moment. I don't think it's handling VisualAddons/VisualAspects correctly at all right now.

mxgrey commented 8 years ago

Oh, it looks like VisualAddon information is handled by GlutWindow rather than OpenGLRenderInterface, so it's not as out of date as I thought.

The OSG implementation is trickier since we need to construct and maintain a scene graph. In my opinion, the main issues with it are:

  1. We have a lot of redundancy between classes. We could probably use templates to eliminate this (but then again I see templates as a solution to everything, so don't feel obligated to use them).
  2. We have a lot of repetitive information. The Single Source of Truth rule is violated all over the place.
  3. We don't have a way of recognizing when a shape's type is changed. That could cause some nasty issues if a user changes out the Shape that's being held by a ShapeFrame.

Edit: Also, if you do decide to make any changes, I would recommend making them on top of the grey/aspects branch of #659 to avoid nasty API conflicts when it comes time to merge.

costashatz commented 8 years ago

I think the OpenGL implementation is even worse off than OSG at the moment.

Hehe! It's good that you removed it from the core library then..

Oh, it looks like VisualAddon information is handled by GlutWindow rather than OpenGLRenderInterface, so it's not as out of date as I thought.

So, what does this mean exactly?

We have a lot of redundancy between classes. We could probably use templates to eliminate this (but then again I see templates as a solution to everything, so don't feel obligated to use them).

I love templates. Check parts of my code here.

We don't have a way of recognizing when a shape's type is changed. That could cause some nasty issues if a user changes out the Shape that's being held by a ShapeFrame.

This is something general as far as I understood from #664 . So, let's leave it aside for the moment.

What is the structure that you want for the renderers? I imagine the following:

// Create skeletons, load URDFs, files etc
WorldPtr world = ...; // initialize world
// add everything to world
SimWindow<OsgRenderer> window(world);
window.simulate(2.0); // this would simulate the world for 2 seconds and display it in SimWindow
// Create skeletons, load URDFs, files etc
WorldPtr world = ...; // initialize world
// add everything to world
SimWindow<OsgRenderer> window(world);
window.show_async(refresh_rate); // display in different thread
while(!end_my_sim)
{
   // do my world loop
   wolrd->step();
   // do post processing of the step
}

I like more the second approach and should not have more coding/tinkering than the first approach. I like this one better, because we can remove all the World updates from the Renderer and leave them to the user. And for more beginner users, we can have helper functions like: window.run_world(time), window.run_world(forever). I am not sure about all the drag n' drop and other things that you have, how they can be properly implemented, but there's definitely a way. I currently do not have any use case where I need any of these tricks and I am not sure if a Kinematics and Dynamics Library should offer them. I would like camera manipulation functionalities and just that, because all the gui code should be for debugging and demos and not for interactive applications. If someone really needs an interactive application, then he would (and he should) definitely develop his one rendering application.

What do you think? Maybe I am completely off topic and too centered on my usage of the library..

mxgrey commented 8 years ago

So, what does this mean exactly?

My last couple posts were somewhat confusing, so allow me to clarify: When I looked at OpenGLRenderInterface it looked as though it wasn't handling any of the VisualAspect information at all. This made me think that it was far behind in terms of compatibility with the recent developments. This is when I made the post saying that I thought the OpenGL code was far behind. Then later I took a look at SimWindow and found that the VisualAspect information was being handled there instead. So the OpenGL code is compatible with the latest developments, despite what I initially said.

This is something general as far as I understood from #664 . So, let's leave it aside for the moment.

This issue is actually not present in the OpenGL render pipeline at the moment. Moreover, the reason it exists in the OSG pipeline is due to a design flaw. If we're redesigning the OSG pipeline, then I think it's worth addressing this at the same time.

OpenGLRenderer and OsgRenderer classes that inherit from Renderer and implement the abstract methods.

I believe pretty strongly that dart::gui::osg does not fit in the RenderInterface pipeline. The reason is that the RenderInterface operates by redrawing each shape from scratch on every iteration whereas OSG operates by constructing a scene graph and simply updating the relative transforms. The OSG implementation is far more efficient than what we could achieve with RenderInterface because we have OSG managing the vertex, primitive, and color buffers within the scene graph. I think we would be handicapping ourselves with no real benefit if we tried to have dart::gui::osg adhere to the same abstract base class as OpenGLRenderInterface.

Moreover, rendering and user interface are coupled with OSG whereas the RenderInterface pipeline exists to decouple those things (giving us the independent classes of OpenGLRenderInterface and GlutWindow). The built-in OSG features like drag-and-drop and "headlights" require these things to be couple.

And finally, an important goal for me with dart::gui::osg is to allow it to be seamlessly integrated into non-DART OSG scene graphs. In other words, if someone has their own OSG scene graph application, I'd like them to be able to plug a dart::gui::osg::WorldNode into their scene graph with no trouble. If we tried to tie dart::gui::osg to the dart::gui::RenderInterface pipeline, that could complicate matters.

SimWindow class with methods like record, simulate(double time) (this should update the world), etc, templated with the actual Renderer (either OpenGL or Osg or a custom one by a user).

If we want a consistent interface between the different GUI classes, I would be okay with a common base class for them. However, I have two concerns about this:

  1. There will inevitably be features available in dart::gui::osg that we can't get with the non-OSG pipeline, because they are already available in OSG, but they would be too costly for us to implement for the non-OSG.
  2. The pipeline for OSG is very different from the non-OSG pipeline, including the way rendering & user interface are coupled or decoupled. This might make it prohibitively difficult (perhaps impossible?) to come up with a consistent interface that works and makes sense for both.

I am not sure about all the drag n' drop and other things that you have ... I am not sure if a Kinematics and Dynamics Library should offer them ... the gui code should be for debugging and demos and not for interactive applications

This is why it's an optional component and largely isolated from the rest of the code base. It used to be a separate library altogether (although kept in the same repo), but once we started using a component pattern for the build system it was decided that we should pull it in as a component. I think drag-and-drop is pretty useful for a broad range of applications, as well as for debugging purposes. Much of my own research is related to teleoperation of robots, so I need these features myself. And I would hate for everyone who needs drag-and-drop to have to reinvent it for their own applications.

We could also make the rendering asynchronous.

I'm open to the idea of asynchronous rendering, but first I think we should incorporate mutexes into the World and/or Skeleton classes. In the past we've discussed how to appropriately handle mutexes in DART, but we never came to a final decision.

In conclusion

I think we'd be hard-pressed to create a coherent shared interface between the different rendering pipelines. I'm certainly not opposed to it if a reasonable and functional design can be proposed, but dart::gui::osg was created separately from the RenderInterface pipeline in the first place because we couldn't see a way for them to fit together. I think fixing the current issues with dart::gui::osg should take a higher priority than trying to put together a common interface.

costashatz commented 8 years ago

I think we'd be hard-pressed to create a coherent shared interface between the different rendering pipelines. I'm certainly not opposed to it if a reasonable and functional design can be proposed, but dart::gui::osg was created separately from the RenderInterface pipeline in the first place because we couldn't see a way for them to fit together.

You are right about this. To be honest I never used too much osg and I forgot how it was organized...

I think fixing the current issues with dart::gui::osg should take a higher priority than trying to put together a common interface.

Yeap.. You are right..! So let's get to them. The first 2 seem easier than the third:

We don't have a way of recognizing when a shape's type is changed. That could cause some nasty issues if a user changes out the Shape that's being held by a ShapeFrame.

Since we know a lot better your library, what do you suggest?

mkoval commented 8 years ago

I've been following this discussion and was about to post exactly the reply that @mxgrey did. I completely agree with everything he said - especially that "we'd be hard-pressed to create a coherent shared interface between the different rendering pipelines."

In addition to the procedural / scene graph distinction that @mxgrey made, there is a third category: out-of-process viewers that communicate via IPC. This is the paradigm used by RViz and web-based renderers (e.g. robot_web_tools, three.js). All three of these use-cases have distinctly different APIs and performance characteristics.

Finally, I was under the impression that we were deprecating the draw functions in dynamics for precisely this reason. Is that no longer the case?

mxgrey commented 8 years ago

I was under the impression that we were deprecating the draw functions in dynamics for precisely this reason.

The primary motivations of deprecating removing it were:

  1. We didn't want dart/dynamics to be dependent on dart/renderer or dart/gui.
  2. Some of the draw() functions were directly dependent on OpenGL instead of using the RenderInterface at all.

But the issue of rendering pipelines being so variable is another good motivation for removing it. The draw() functions are gone now, although we still have the RenderInterface class. I imagine we'll be holding onto that at least until we have an implementation based on glfw or anything else besides the outdated glut. Then again, we could potentially keep the RenderInterface class indefinitely so it can be shared by all procedural-style rendering pipelines; I wouldn't mind either way.

mxgrey commented 7 years ago

The fix for this is merged. Please feel free to reopen if you still encounter issues.