OSG shadows #978

costashatz · 2018-02-09T13:15:49Z

This is a better version of #976, where:

I changed the dart::gui::osg::Viewer class to be able to enable/disable shadows. Edit: I mean that now there's no ShadowedWorldNode and you can enable/disable shadows directly from the viewer
Edit: I also added the possibility of selecting a ShadowTechnique from OSG (not all of them though, because some of them require complex settings)
I did not touch the lights (this should be in a separate PR)

There are two issues (one minor and one major):

For now we select the mLight1 as the light that casts shadows (because this one is higher in the up direction). I am not an OSG expert and I do not know how to combine multiple lights to produce shadows. I think this is minor as it should mostly be ok. Here's one example:
The ImGuiViewer is not working properly once you enable shadows (either black or distorted views on the ImGui panels). Here's an example:

I am investigating why this happens and I will try to provide a fix..

@jslee02 let me know what you think..

codecov · 2018-02-11T00:58:28Z

Codecov Report

Merging #978 into release-6.4 will not change coverage.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           release-6.4    #978   +/-   ##
===========================================
  Coverage         56.7%   56.7%           
===========================================
  Files              310     310           
  Lines            23958   23958           
===========================================
  Hits             13585   13585           
  Misses           10373   10373

Impacted Files	Coverage Δ
dart/dynamics/ShapeFrame.hpp	`80% <ø> (ø)`	⬆️
dart/dynamics/detail/ShapeFrameAspect.hpp	`88.88% <ø> (ø)`	⬆️
dart/dynamics/ShapeFrame.cpp	`65.85% <100%> (ø)`	⬆️

jslee02

This generally looks good to me! Thanks for making this change.

I see several inconsistent code style in this change with the DART's code style, but it would be good to resolve it using clang-format once the style related PRs (#979, #981) are merged.

In general, I would prefer using a setting function for shadow technique instead of using enum for better flexibility (e.g., Viewer::setShadowTechnique(osg::ref_ptr<osgShadow::ShadowTechnique> newShadowTechnique)); with the current design, we have to change the code to use a new shadow technique. However, I'm okay with the current design because I don't expect OSG will add new techniques soon.

@mxgrey It would be nice to hear from you since OSG GUI was introduced by you! 😛

costashatz · 2018-02-12T09:52:18Z

This generally looks good to me! Thanks for making this change.

Thanks for the quick feedback.

I see several inconsistent code style in this change with the DART's code style, but it would be good to resolve it using clang-format once the style related PRs (#979, #981) are merged.

OK. Let's let clang-format do its job! 😄

In general, I would prefer using a setting function for shadow technique instead of using enum for better flexibility (e.g., Viewer::setShadowTechnique(osg::ref_ptrosgShadow::ShadowTechnique newShadowTechnique)); with the current design, we have to change the code to use a new shadow technique. However, I'm okay with the current design because I don't expect OSG will add new techniques soon.

I can do this change later today..

costashatz · 2018-02-12T15:11:43Z

@jslee02 I followed your advice about setting the shadow technique. I also had to expose the light sources of the viewer so that someone can use them in the shadow techniques (some of them recommend/require to set which light you want to use for shadowing).

costashatz · 2018-02-13T14:48:34Z

@jslee02 @mxgrey I cannot seem to find a solution for the ImGuiViewer. Is it okay if we put a warning (or even disable the enableShadows function for ImGuiViewer) and merge it like this?

jslee02 · 2018-02-13T17:14:23Z

@costashatz Looking at the code, I found that there is another issue that InteractiveFrame is also under the shadow scene, which shouldn't be. I think we need more than just mRootGroup-mPhysicsGroup distinction. Let me try to come up with a possible solution (hopefully for the ImGUI issue as well).

mxgrey · 2018-02-13T17:57:45Z

We could add a flag to the VisualAspect class for enabling and disabling shadows. The InteractiveFrame could disable its own shadow flags by default.

mxgrey · 2018-02-13T18:01:52Z

To complement the shadow flag, we would probably need to add an update function to the Viewer class to tell it to re-examine the shadow flags, so it can re-sort the frames between the shadowed group and the root group.

mxgrey · 2018-02-13T18:09:27Z

dart/gui/osg/Viewer.hpp

  /// Constructor for dart::gui::osg::Viewer. This will automatically create the
  /// default event handler.
-  Viewer(const ::osg::Vec4& clearColor = ::osg::Vec4(0.9,0.9,0.9,1.0));
+  Viewer(const ::osg::Vec4& clearColor = ::osg::Vec4(0.9,0.9,0.9,1.0), bool shadowsOn = false, ::osg::ref_ptr<osgShadow::ShadowTechnique> shadowTechnique = nullptr);


Would it make sense to get rid of the shadowsOn flag and allow a nullptr for shadowTechnique to indicate that shadows should be off? Then if someone passes in a non-null value for shadowTechnique, we have it default to shadows being on?

Also, if there's a default native osgShadow::ShadowTechnique that we can recommend to users, it would be nice if we referred to it in the documentation of this function.

mxgrey · 2018-02-13T18:11:54Z

dart/gui/osg/Viewer.hpp

+  const ::osg::ref_ptr<::osg::Group>& getPhysicsGroup() const;
+
+  bool isShadowed() const;
+  void enableShadows(bool _enable = true, ::osg::ref_ptr<osgShadow::ShadowTechnique> shadowTechnique = nullptr);


These default values seem very counter-intuitive to me. Is it possible to have shadows with a nullptr for the osgShadow::ShadowTechnique? I would assume that providing a nullptr will make it impossible for OSG to render shadows, so it wouldn't make sense to have them enabled. Or is the shadowTechnique something extra that can be applied on top of the default OSG shadowing?

These default values seem very counter-intuitive to me. Is it possible to have shadows with a nullptr for the osgShadow::ShadowTechnique? I would assume that providing a nullptr will make it impossible for OSG to render shadows, so it wouldn't make sense to have them enabled. Or is the shadowTechnique something extra that can be applied on top of the default OSG shadowing?

You need the ShadowTechnique in order to have shadows..

Right, which is why this API seems deceptive. It's simultaneously telling the Viewer "I want you to enable shadows" and "I don't want you to have a ShadowTechnique".

I understand now that we're interpreting a nullptr as "use some default ShadowTechnique", but that isn't clear without digging into the source code of this function.

mxgrey · 2018-02-13T18:16:57Z

dart/gui/osg/Viewer.cpp

+  mShadowed = _enable;
+}
+
+void Viewer::setShadowTechnique(::osg::ref_ptr<osgShadow::ShadowTechnique> shadowTechnique) {


I see now that if the user passes a nullptr, we replace it with a default osgShadow::ShadowMap.

I think that's a bit opaque, so what if we instead make an osg::ref_ptr<osgShadow::ShadowMap> instance the default value for these function arguments?

I'm further realizing that the initialization of the ShadowMap is using one of the member variables of the Viewer class, mLight1.

Maybe an alternative approach would be to make overloads of the functions that take no ShadowTechnique argument, and then document to the user that it will create a ShadowMap by default, using the mLight1 member of the Viewer.

I think that's a bit opaque, so what if we instead make an osg::ref_ptrosgShadow::ShadowMap instance the default value for these function arguments?

Yes we can do this..

I'm further realizing that the initialization of the ShadowMap is using one of the member variables of the Viewer class, mLight1.

That's why I exposed the LightSources so that users can define their own ShadowTechniques and be able to choose the light source that will cast the shadows. We can't do it generically because not all of the techniques have the setLight() functionality..

costashatz · 2018-02-13T18:59:46Z

@costashatz Looking at the code, I found that there is another issue that InteractiveFrame is also under the shadow scene, which shouldn't be. I think we need more than just mRootGroup-mPhysicsGroup distinction. Let me try to come up with a possible solution (hopefully for the ImGUI issue as well).

Hmm.. I didn't pay attention to the InteractiveFrame.. Let me also try to think about it..

We could add a flag to the VisualAspect class for enabling and disabling shadows. The InteractiveFrame could disable its own shadow flags by default.

We cannot do this, because as OSG docs say: "Not all of the techniques respect the shadowing flags". In practice, most of them ignore at least one flag (either caster or receiver of shadow).!

mxgrey · 2018-02-13T21:51:22Z

We cannot do this, because as OSG docs say: "Not all of the techniques respect the shadowing flags"

When I say "shadow flag" I'm just talking about a simple boolean within the DART API. It would be agnostic towards the underlying rendering engine. The shadowing flag of the VisualAspect would be checked by the dart-gui-osg wrapper, and the wrapper would decide whether to put the object in the group that gets affected by shadows (mPhysicsGroup) or the group that does not get affected by shadows (mRootGroup).

We could even set it up so that every rendering loop it checks whether that flag has changed for each ShapeFrame, and then switches the frame into the other group if it has changed.

This assumes that my understanding is correct: Only objects in mPhysicsGroup will be shaded. Is that correct?

costashatz · 2018-02-13T23:15:13Z

This assumes that my understanding is correct: Only objects in mPhysicsGroup will be shaded. Is that correct?

Yes.. At least that's what is said by the OSG docs and matches what I tried so far...

costashatz · 2018-02-14T13:08:36Z

When I say "shadow flag" I'm just talking about a simple boolean within the DART API. It would be agnostic towards the underlying rendering engine. The shadowing flag of the VisualAspect would be checked by the dart-gui-osg wrapper, and the wrapper would decide whether to put the object in the group that gets affected by shadows (mPhysicsGroup) or the group that does not get affected by shadows (mRootGroup).

We could even set it up so that every rendering loop it checks whether that flag has changed for each ShapeFrame, and then switches the frame into the other group if it has changed.

I like this idea. Let me try to see what happens..

costashatz · 2018-02-15T15:22:18Z

I implemented @mxgrey 's idea and it works nicely for the InteractiveFrame. I had to move everything to the WorldNode instead of the Viewer. Later today I will put a check to see if any frame changed its shadow flag and put some documentation.

I didn't manage to make ImGuiViewer work properly... I am investigating a bit more to see if I can find anything..

costashatz · 2018-02-15T22:15:58Z

After some more investigation, I think that we need to create a version of ImGuiViewer that uses OpenSceneGraph's primitives (which is certainly possible as ImGui just gives us a list of indexed primitives to draw).. Personally, I don't think I have the time nor the expertise (I am not using this part of the API at all) to do that.. If you agree, we can do one of the following (once you review the rest of the changes):

Disable completely the shadows when using ImGuiViewer (we can also give a silent warning to the user wanting to use shadows with it)..
Leave it as is and put a warning when a user uses shadows with ImGuiViewer...
Post-pone the merge of this PR until we find a solution..

Personally I prefer either of the 2 first options because I am not using the ImGuiViewer at all and if it is relatively well documented, it should be ok for most users. Of course, we should make an issue for creating a version of ImGuiViewer with OpenSceneGraph primitives that can be attached to the scene graph.

@jslee02 @mxgrey let me know what you think..

mxgrey · 2018-02-15T22:21:50Z

I'm okay with either of the first two options.

I wouldn't consider this (nor ImGui) to be a critical feature of DART, so correctness and compatibility between the two isn't a high priority. Shadows are definitely a desirable feature, and it's mostly unrelated to the ImGui feature, so I don't see any reason to block it.

mxgrey · 2018-02-15T22:27:00Z

dart/gui/osg/WorldNode.hpp

@@ -67,7 +68,8 @@ class WorldNode : public ::osg::Group
  friend class Viewer;

  /// Default constructor
-  explicit WorldNode(std::shared_ptr<dart::simulation::World> _world = nullptr);
+  /// Shadows are disabled by default
+  explicit WorldNode(std::shared_ptr<dart::simulation::World> _world = nullptr, ::osg::ref_ptr<osgShadow::ShadowTechnique> shadowTechnique = nullptr);


I'm still a little bit leery about having shadowTechnique = nullptr secretly mean "Let us choose a default shading technique for you".

However, we do already have a precedent of this pattern where passing in a nullptr for ResourceRetriever means that we will use a default LocalResourceRetriever.

I might feel better about this pattern if we rename the argument from shadowTechnique to something like alternativeTechnique. That will at least tell the user that they are specifying an alternative rather than specifying the technique to use. Then passing a nullptr naturally says "I don't want to use an alternative", and so we apply the default shading technique.

Sorry if this sounds petty, but without something along these lines, a user would have to look into the source code to actually understand how these functions should be used.

Sorry if this sounds petty, but without something along these lines, a user would have to look into the source code to actually understand how these functions should be used.

No worries. I understand what you say. Nevertheless, in this version of the code, nullptr means no shadow (not a default shadow behavior).. The user has to specify their own ShadowTechnique. I could create a static default technique if you like..

Oh, sorry, I didn't notice that part of the recent change.

I actually liked providing a default shadow technique; I just wasn't comfortable with the semantics of "giving a nullptr technique means we should use a default". Sorry for not communicating that well.

I'm not clear on how a static default would work, since it seems to need information about the Viewer that's using it. I guess we could make it a factory function that takes a Viewer* argument and returns a ref_ptr<ShadowTechnique>? We wouldn't be able to use that function as the default argument for this constructor, but it could at least make it easy for users to enable shadows. We could add a comment in the documentation of this function that refers to the default ShadowTechnique factory.

Since the shadows conflict with the ImGui feature, I suppose we should have shadows turned off by default anyway. It would've been nice to have shadows on by default, but we can tackle that later once we've fixed the ImGui conflict.

I'm not clear on how a static default would work, since it seems to need information about the Viewer that's using it. I guess we could make it a factory function that takes a Viewer* argument and returns a ref_ptr? We wouldn't be able to use that function as the default argument for this constructor, but it could at least make it easy for users to enable shadows. We could add a comment in the documentation of this function that refers to the default ShadowTechnique factory.

That's exactly what I had in mind.. A static member function that creates a default ShadowTechnique (given a Viewer*) that users can use.. I'll do that asap..

ince the shadows conflict with the ImGui feature, I suppose we should have shadows turned off by default anyway. It would've been nice to have shadows on by default, but we can tackle that later once we've fixed the ImGui conflict.

I agree on this and that's the reason I left the shadows disabled by default.. 😄

I was also thinking of modifying the osgOperationalSpaceControl example to be able to enable/disable shadows interactively (with a simple event handler). What do you think?

That sounds like a great place to put an example of this, thanks!

jslee02 · 2018-02-15T23:10:13Z

I'm also fine with any of the first two options!

costashatz · 2018-02-15T23:42:36Z

I went with the second option (warning when enabling shadows in ImGuiViewer). I also added an example of enabling/disabling shadows in osgOperationalSpaceControl example. Lastly, I ran make check-format and I got the following results:

Scanning dependencies of target check-format
Checking 4 files... 
Every file seems to comply with our code convention.
Done.
Built target check-format

So I guess the styling should be ok!

@jslee02 @mxgrey let me know if you have anything to add or you want me to fix something.

jslee02 · 2018-02-16T18:14:42Z

@costashatz Regarding the style, we're just checking four files in lcpsolver directory as an example. That said your changes are not checked. 😁 We will format the code for the entire code eventually.

The current change looks good to me. I think we can merge this on approval of @mxgrey.

mxgrey · 2018-02-16T18:18:25Z

I'll do one last careful review tonight as soon as I get the chance, but it's looking very good to me as well.

jslee02

Left some nitpick comments

jslee02 · 2018-02-16T18:18:39Z

dart/gui/osg/Viewer.hpp

+  /// Get one of the LightSources of this Viewer
+  /// index either 0 or 1
+  /// Useful for shadowing techniques
+  const ::osg::ref_ptr<::osg::LightSource>& getLightSource(unsigned int index = 0) const;


Nit: Please use std::size_t for index parameters.

jslee02 · 2018-02-16T18:19:45Z

dart/gui/osg/WorldNode.hpp

@@ -67,7 +68,8 @@ class WorldNode : public ::osg::Group
  friend class Viewer;

  /// Default constructor
-  explicit WorldNode(std::shared_ptr<dart::simulation::World> _world = nullptr);
+  /// Shadows are disabled by default
+  explicit WorldNode(std::shared_ptr<dart::simulation::World> _world = nullptr, ::osg::ref_ptr<osgShadow::ShadowTechnique> shadowTechnique = nullptr);


Nit: Please remove the leading underscore from _world.

mxgrey

I had one nitpick, but it's not nearly important enough to block on.

Thanks, @costashatz, for this excellent (and frequently requested) addition to DART!

mxgrey · 2018-02-17T02:32:20Z

dart/gui/osg/WorldNode.cpp

+  shadowedScene->setReceivesShadowTraversalMask(ReceivesShadowTraversalMask);
+  shadowedScene->setCastsShadowTraversalMask(CastsShadowTraversalMask);
+
+  // set the physics group


Nitpick: This is called normal group now rather than physics group.

costashatz and others added 4 commits February 9, 2018 13:32

Shadows in OpenSceneGraph - Part I

70b6d2f

Shadows in OpenSceneGraph - Part II: multiple techniques

a4b6cb4

Shadows in OpenSceneGraph - minor fix

6559528

Merge branch 'release-6.4' into shadows

3747652

jslee02 approved these changes Feb 11, 2018

View reviewed changes

jslee02 added the pr: need review label Feb 11, 2018

jslee02 added this to the DART 6.4.0 milestone Feb 12, 2018

costashatz added 2 commits February 12, 2018 12:41

@jslee02 comments

53b8a05

Merge branch 'shadows' of github.com:costashatz/dart into shadows

1fd3aec

Merge branch 'release-6.4' into shadows

6a3c530

Merge branch 'release-6.4' into shadows

104e0e4

jslee02 added the help wanted Indicates wanting help on an issue or pull request label Feb 13, 2018

mxgrey reviewed Feb 13, 2018

View reviewed changes

jslee02 modified the milestones: DART 6.4.0, DART 6.5.0 Feb 13, 2018

costashatz added 2 commits February 15, 2018 14:21

Updated .gitignore

ab72b9d

Shadowing capabilities go to WorldNode/Addig flag in VisualAspect

47b7e41

Added check for changing shadow flag and docs

a996028

mxgrey reviewed Feb 15, 2018

View reviewed changes

Helper function to create default osg shadow: ShadowMap

62bb1ef

costashatz added 2 commits February 16, 2018 00:34

Add warning when using shadows in ImGuiViewer

68dc5db

Add example of shadows in osgOperationalSpaceControl

5ddba86

costashatz mentioned this pull request Feb 15, 2018

Restructure NOSALRO/robot_dart#1

Merged

11 tasks

Merge branch 'release-6.4' into shadows

d7b83e8

jslee02 modified the milestones: DART 6.5.0, DART 6.4.0 Feb 16, 2018

jslee02 reviewed Feb 16, 2018

View reviewed changes

Minor comments

c05a17f

mxgrey approved these changes Feb 17, 2018

View reviewed changes

costashatz and others added 2 commits February 17, 2018 10:25

Minor typo in WorldNode

d0d3b82

Merge branch 'release-6.4' into shadows

6ec3005

jslee02 added status: ready to merge and removed help wanted Indicates wanting help on an issue or pull request status: review needed labels Feb 17, 2018

Update changelog for dartsim#978

568631e

jslee02 merged commit a673c87 into dartsim:release-6.4 Feb 17, 2018

jslee02 mentioned this pull request May 10, 2019

Remove warning of ImGuiViewer + OSG shadow #1312

Merged

5 tasks

OSG shadows #978

OSG shadows #978

Conversation

costashatz commented Feb 9, 2018 • edited Loading

codecov bot commented Feb 11, 2018 • edited Loading

Codecov Report

jslee02 left a comment

Choose a reason for hiding this comment

costashatz commented Feb 12, 2018

costashatz commented Feb 12, 2018

costashatz commented Feb 13, 2018

jslee02 commented Feb 13, 2018

mxgrey commented Feb 13, 2018

mxgrey commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxgrey Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

mxgrey Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costashatz commented Feb 13, 2018 • edited Loading

mxgrey commented Feb 13, 2018

costashatz commented Feb 13, 2018

costashatz commented Feb 14, 2018

costashatz commented Feb 15, 2018

costashatz commented Feb 15, 2018

mxgrey commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 commented Feb 15, 2018

costashatz commented Feb 15, 2018

jslee02 commented Feb 16, 2018

mxgrey commented Feb 16, 2018

jslee02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxgrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costashatz commented Feb 9, 2018 •

edited

Loading

codecov bot commented Feb 11, 2018 •

edited

Loading

mxgrey Feb 13, 2018 •

edited

Loading

mxgrey Feb 13, 2018 •

edited

Loading

costashatz commented Feb 13, 2018 •

edited

Loading