Expose shadow texture size for directional lighting in SDF #1034

athenaz2 · 2024-08-12T23:03:23Z

🎉 New feature

Summary

Shadows appear jagged in some scenes. It would be nice for them to look smoother and sharper. Investigated increasing shadow texture size - it seems that as long as there isn't too many lights, it does not have a significant impact on VRAM.

Thus, added ability to set shadow texture size (currently, only for directional lights) within the MinimalScene plugin, and for Ogre2. The default shadow texture size is 2K, but setting it to a higher value (4K, 8K, etc.) allows for sharper shadows.

Currently supporting 1K, 2K, 4K, 8K, and 16K. So in SDF file, the respective values 1024, 2048, 4096, 8192, and 16384. In the future, can look to enable setting shadow texture size for other light types (spot, point lights).

Also, if using OpenGL, ensured that max shadow texture size can use GL_MAX_TEXTURE_SIZE instead of hardcoding it to 8K.

Depot scene from Edifice demo (SDF attached below). Note that scene has 1 directional light (darker shadow) and 1 point light (lighter shadow).

Shadow texture size of 2K for directional light

Shadow texture size of 16K for directional light

Test it

depot_twolights.sdf.txt
shapes.sdf.txt

Modify the param in the SDF.

<shadows>
  <texture_size light_type="directional">8192</texture_size>
</shadows>

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

athenaz2 · 2024-08-12T23:05:16Z

Shapes demo (only 1 directional light).

Shadow texture size of 2K for directional light

Shadow texture size of 8K for directional light

iche033 · 2024-08-12T23:36:53Z

ogre2/src/Ogre2Scene.cc

@@ -15,6 +15,8 @@
 *
 */

+#include <GL/gl.h>


you'll need to use ifdef here as the file path is different on different platforms, see

gz-rendering/ogre2/src/Ogre2Marker.cc

Lines 18 to 26 in f606a32

#ifdef __APPLE__

#define GL_SILENCE_DEPRECATION

#include <OpenGL/gl.h>

#include <OpenGL/glext.h>

#else

#ifndef _WIN32

#include <GL/gl.h>

#endif

#endif

Added it here - 3b1bc7a

include/gz/rendering/Light.hh

iche033 · 2024-08-12T23:39:22Z

include/gz/rendering/Scene.hh

+                  unsigned int _textureSize) = 0;
+
+      /// @brief \brief Get the shadow texture size for the given light type.
+      /// @param _lightType Light type that creates the shadow


nit: replace @ with \ for consistency with other doxygen comments

iche033 · 2024-08-12T23:43:10Z

ogre2/src/Ogre2Scene.cc

+  if (_lightType == LightType::LT_DIRECTIONAL)
+  {
+    this->dataPtr->dirTexSize = _textureSize;
+  }


move this block to the top of the function so that we just return early if it's not a supported light type. Also add an else block with gzwarn msg if user specifies spot or point lights.

3b1bc7a. I also have a check for it in MinimalScene too

iche033 · 2024-08-12T23:46:28Z

ogre2/src/Ogre2Scene.cc

+unsigned int Ogre2Scene::ShadowTextureSize(LightType _lightType) const
+{
+  // todo: return based on light type, currently only dir light is supported
+  if (_lightType)


you can also make spotPointTexSize a member variable so that you can support returning point and spot light shadow texture size here. The user just won't be able to set them yet.

then you can use switch to check _lightType and return the corresponding texture size.

iche033 · 2024-08-12T23:49:46Z

src/base/BaseScene.cc

+//////////////////////////////////////////////////
+unsigned int BaseScene::ShadowTextureSize(LightType _lightType) const
+{
+  return this->ShadowTextureSize(_lightType);


you can just print an error here:

gzerr << "Shadow texture size not supported by: " << this->Engine()->Name() << std::endl;

ogre2 implements this function so the user should not see the error msg since it should call the ShadowTextureSize from the derived class

iche033 · 2024-08-12T23:50:45Z

ogre2/src/Ogre2Scene.cc

+    unsigned int _textureSize)
+{
+  // If _textureSize exceeds max possible tex size, then use default
+  if (_textureSize > this->dataPtr->maxTexSize / 2)


just checking, what's the reason for the divide by 2?

My maxTexSize is 32K, and I allow texsizes (that are pow of 2 vals) up to 16K. If I set _textureSize to 32K this happens:

The pitch-black shadow here is from the point light. Compared to the images attached to original PR post, the shadow created by directional light is gone. I think this is maybe because the math on atlasResolution went crazy. (doesn't fit within atlas?)

I see. I tested with 16K max tex size as that's what my machine supports and it works fine. So how about instead of dividing by 2, we set a hard cap on the max tex size to 16K? I remembered and found this comment mentioning the 16K texture size cap as well.

You can do this above when setting this->dataPtr->maxTexSize, e.g.

this->dataPtr->maxTexSize = std::min(glMaxTexSize, 16384u)

and add a todo note mentioning that there are issues with dir shadows when setting to a number larger than 16K.

Capped maxTexSize to 16K in ddde4d9, added TODO comment in a8a3b65

iche033 · 2024-08-12T23:57:37Z

ogre2/src/Ogre2Scene.cc

+  bool validTexSize = std::find(texSizeOptions.begin(),
+      texSizeOptions.end(), _textureSize)
+      != texSizeOptions.end() ? true : false;
+  if (!validTexSize)


instead of generating a list of possible tex size options, I think you can simplify this check to

if (_textureSize >= 1024u && math::isPowerOfTwo(_textureSize)?

Yeah this is a good idea. 3b1bc7a

iche033 · 2024-08-13T00:07:42Z

you'll need to retarget this to main because the changes break ABI

iche033 · 2024-08-15T01:11:48Z

ogre2/src/Ogre2Scene.cc

+    unsigned int _textureSize)
+{
+  // If _textureSize exceeds max possible tex size, then use default
+  if (_textureSize > this->dataPtr->maxTexSize / 2)


I see. I tested with 16K max tex size as that's what my machine supports and it works fine. So how about instead of dividing by 2, we set a hard cap on the max tex size to 16K? I remembered and found this comment mentioning the 16K texture size cap as well.

You can do this above when setting this->dataPtr->maxTexSize, e.g.

this->dataPtr->maxTexSize = std::min(glMaxTexSize, 16384u)

and add a todo note mentioning that there are issues with dir shadows when setting to a number larger than 16K.

ogre2/src/Ogre2Scene.cc

iche033 · 2024-08-15T01:15:16Z

include/gz/rendering/Light.hh

+    enum class GZ_RENDERING_VISIBLE LightType
+    {
+      /// \brief No light type specified
+      EMPTY = 0,


was there an issue with what you had before, LT_EMPTY?

Changing LightType to enum class ensures any usage of it is like rendering::LightType::DIRECTIONAL (or other light type values) as opposed to rendering::LT_DIRECTIONAL, so no need for LT_ in the naming anymore.

iche033 · 2024-08-15T01:16:49Z

ogre2/src/Ogre2Scene.cc

+  }
+
+  // if _textureSize is an invalid texture size, then use default
+  if (_textureSize < 1024u || _textureSize > (this->dataPtr->maxTexSize / 2)


I think it should be fine to set the lower limit to 512?

…point lights Signed-off-by: Athena Z <[email protected]>

Signed-off-by: Athena Z <[email protected]>

…exSize can use GL_MAX_TEXTURE_SIZE instead of currently hardcoded val Signed-off-by: Athena Z <[email protected]>

Signed-off-by: Athena Z <[email protected]>

Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Athena Z. <[email protected]> Signed-off-by: Athena Z <[email protected]>

Signed-off-by: Athena Z <[email protected]>

…e to return bool Signed-off-by: Athena Z <[email protected]>

Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Athena Z. <[email protected]> Signed-off-by: Athena Z <[email protected]>

Signed-off-by: Athena Z <[email protected]>

codecov · 2024-08-15T22:53:16Z

Codecov Report

Attention: Patch coverage is 57.40741% with 23 lines in your changes missing coverage. Please review.

Project coverage is 75.63%. Comparing base (e187f52) to head (a8a3b65).
Report is 52 commits behind head on main.

Files	Patch %	Lines
ogre2/src/Ogre2Scene.cc	70.45%	13 Missing ⚠️
src/base/BaseScene.cc	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1034      +/-   ##
==========================================
- Coverage   75.68%   75.63%   -0.06%     
==========================================
  Files         177      177              
  Lines       16959    16998      +39     
==========================================
+ Hits        12835    12856      +21     
- Misses       4124     4142      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Athena Z <[email protected]>

iche033

looks good to me!

athenaz2 requested a review from iche033 as a code owner August 12, 2024 23:03

github-actions bot added the 🎵 harmonic Gazebo Harmonic label Aug 12, 2024

iche033 reviewed Aug 13, 2024

View reviewed changes

athenaz2 changed the base branch from gz-rendering8 to main August 13, 2024 16:47

athenaz2 mentioned this pull request Aug 13, 2024

Expose shadow texture size for directional lighting in SDF gazebosim/gz-gui#633

Merged

9 tasks

iche033 reviewed Aug 15, 2024

View reviewed changes

athenaz2 and others added 12 commits August 15, 2024 14:49

Increased texSize only for dir lights and kept texSize same for spot/…

51acbb5

…point lights Signed-off-by: Athena Z <[email protected]>

Expose texsize in sdf

44b1f53

Signed-off-by: Athena Z <[email protected]>

For clarity, renamed func/vars. Also if using OpenGL API, ensure maxT…

3775499

…exSize can use GL_MAX_TEXTURE_SIZE instead of currently hardcoded val Signed-off-by: Athena Z <[email protected]>

Add LightType enum to Light, and also as func param

e33d973

Signed-off-by: Athena Z <[email protected]>

If tex size from SDF is not valid, still use default (do not set it)

eaf1300

Signed-off-by: Athena Z <[email protected]>

Added getter func

896efae

Signed-off-by: Athena Z <[email protected]>

Cleaned up style with code check, added unit tests

693cb12

Signed-off-by: Athena Z <[email protected]>

Update include/gz/rendering/Light.hh

b258c8a

Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Athena Z. <[email protected]> Signed-off-by: Athena Z <[email protected]>

Modify get/set ShadowTextureSize funcs, use ifdef for file paths

e62b161

Signed-off-by: Athena Z <[email protected]>

Changed LightType from enum to enum class, modify SetShadowTextureSiz…

640d0b2

…e to return bool Signed-off-by: Athena Z <[email protected]>

Update ogre2/src/Ogre2Scene.cc

b369153

Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Athena Z. <[email protected]> Signed-off-by: Athena Z <[email protected]>

Hard cap maxTexSize to 16K and modify logic for checking tex size inputs

ddde4d9

Signed-off-by: Athena Z <[email protected]>

athenaz2 force-pushed the athenaz/texsize_in_sdf branch from a5a6d3a to ddde4d9 Compare August 15, 2024 21:56

Add TODO note re issue setting texsize to val larger than 16K

a8a3b65

Signed-off-by: Athena Z <[email protected]>

iche033 and others added 2 commits August 16, 2024 11:29

Merge branch 'main' into athenaz/texsize_in_sdf

091bac6

Add unit tests

eca40a8

Signed-off-by: Athena Z <[email protected]>

iche033 approved these changes Aug 16, 2024

View reviewed changes

iche033 merged commit 8edd14f into gazebosim:main Aug 16, 2024
8 of 9 checks passed

azeey added beta Targeting beta release of upcoming collection 🏛️ ionic Gazebo Ionic and removed 🎵 harmonic Gazebo Harmonic labels Aug 16, 2024

azeey mentioned this pull request Aug 31, 2024

Add Github Action for Windows and fix build #1049

Merged

8 tasks

athenaz2 mentioned this pull request Sep 5, 2024

Add tutorial + example SDF for shadow texture size gazebosim/gz-sim#2597

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose shadow texture size for directional lighting in SDF #1034

Expose shadow texture size for directional lighting in SDF #1034

athenaz2 commented Aug 12, 2024 •

edited

Loading

athenaz2 commented Aug 12, 2024

iche033 Aug 12, 2024

athenaz2 Aug 14, 2024

iche033 Aug 12, 2024

athenaz2 Aug 14, 2024

iche033 Aug 12, 2024

athenaz2 Aug 14, 2024

iche033 Aug 12, 2024

athenaz2 Aug 14, 2024

iche033 Aug 12, 2024

athenaz2 Aug 14, 2024

iche033 Aug 12, 2024

athenaz2 Aug 13, 2024 •

edited

Loading

iche033 Aug 15, 2024

athenaz2 Aug 15, 2024

iche033 Aug 12, 2024

athenaz2 Aug 14, 2024

iche033 commented Aug 13, 2024

iche033 Aug 15, 2024

iche033 Aug 15, 2024

athenaz2 Aug 15, 2024

iche033 Aug 15, 2024

athenaz2 Aug 15, 2024

codecov bot commented Aug 15, 2024

iche033 left a comment

	#ifdef __APPLE__
	#define GL_SILENCE_DEPRECATION
	#include <OpenGL/gl.h>
	#include <OpenGL/glext.h>
	#else
	#ifndef _WIN32
	#include <GL/gl.h>
	#endif
	#endif

Expose shadow texture size for directional lighting in SDF #1034

Expose shadow texture size for directional lighting in SDF #1034

Conversation

athenaz2 commented Aug 12, 2024 • edited Loading

🎉 New feature

Summary

Test it

Checklist

athenaz2 commented Aug 12, 2024

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

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

athenaz2 Aug 13, 2024 • edited Loading

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

iche033 commented Aug 13, 2024

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

codecov bot commented Aug 15, 2024

Codecov Report

iche033 left a comment

Choose a reason for hiding this comment

athenaz2 commented Aug 12, 2024 •

edited

Loading

athenaz2 Aug 13, 2024 •

edited

Loading