Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CustomPSSMShadowCamera: support custom projection matrix #3249

Merged

Conversation

mogumbo
Copy link
Contributor

@mogumbo mogumbo commented Aug 9, 2022

PSSM shadows only support a regular camera matrix defined by horizontal_fov. If you instead use a custom matrix defined by camera intrinsics, the PSSM algorithm does not build a new shadow camera matrix for each of the 3 shadow maps. This PR fixes that deficiency.

Example camera using horizontal_fov:

           <camera>
            <horizontal_fov>0.28</horizontal_fov>
            <image>
              <width>2048</width>
              <height>2048</height>
            </image>
            <clip>
              <near>0.1</near>
              <far>500</far>
            </clip>
          </camera>

Example camera using intrinsics:

           <camera>
            <lens>
              <intrinsics>
                <fx>1434.84</fx>
                <fy>1435.36</fy>
                <cx>1010.23</cx>
                <cy>1034.08</cy>
              </intrinsics>
            </lens>
            <image>
              <width>2048</width>
              <height>2048</height>
            </image>
            <clip>
              <near>0.1</near>
              <far>500</far>
            </clip>
          </camera>

@scpeters scpeters changed the title Fix deficiency when using PSSM with a custom camera matrix CustomPSSMShadowCamera: support custom projection matrix Aug 17, 2022
@jacobperron jacobperron self-requested a review August 17, 2022 20:11
@@ -624,6 +649,10 @@ void CustomPSSMShadowCameraSetup::getShadowCamera(const Ogre::SceneManager *_sm,
// restore near/far
cam->setNearClipDistance(oldNear);
cam->setFarClipDistance(oldFar);
if (cam->isCustomProjectionMatrixEnabled())
{
cam->setCustomProjectionMatrix(true, oldCustomProjMat);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like we're resetting the custom projection matrix back to the old matrix. So we only want to modify the custom projection matrix temporarily (before calling the clip distance methods)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. In the case of non-custom projection matrices, lines 625 and 626 build a new temporary matrix and lines 650 and 651 rebuild the original matrix. Those lines have no effect on a custom matrix, so I found a different way to build a temporary matrix and restore the original.

Line 619 points out that this is all very "hacky," so the original author of this code probably would have preferred a cleaner solution but didn't want all the refactoring that it would have required.

@@ -464,6 +464,16 @@ void CustomPSSMShadowCameraSetup::calculateShadowMappingMatrix(
}
}

//////////////////////////////////////////////////
Ogre::Matrix4 CustomPSSMShadowCameraSetup::buildSimplePerspectiveMatrix(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this function is only used within this translation unit, and it does not depend on member variables, I suggest only declaring it in this .cc file as a static free function, e.g.

static Ogre::Matrix4 buildSimplePerspectiveMatrix(const Ogre::Real _near, const Ogre::Real _far)
{
  ...

And then we don't need to modify the header file.

Copy link
Contributor Author

@mogumbo mogumbo Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that. Out of curiosity, does adding this as a method break and project rules? I don't believe it will break ABI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that. Out of curiosity, does adding this as a method break and project rules? I don't believe it will break ABI.

since it is not a virtual function, it would not break ABI to add it to the installed header file, but keeping it in the .cc file helps limit our API surface area

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@scpeters scpeters merged commit 263772e into gazebosim:gazebo11 Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants