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

Workaround for wrong distortion material applied to some cameras #3136

Merged

Conversation

mogumbo
Copy link
Contributor

@mogumbo mogumbo commented Nov 24, 2021

When using stereo cameras with different distortion parameters and attaching other Ogre compositors to the cameras, the same distortion parameters will be applied to both cameras. This PR implements a workaround.

It looks to me like Distortion.cc tries to do the right thing by using a unique Ogre::CompositorInstance for each camera. Unfortunately, it looks like these CompositorInstances share the same CompositionTechnique inside Ogre, causing passes and materials to get stomped from one CompositorInstance to the next. This stomping only happens if you add compositors to your cameras in addition to the distortion compositor.

// to this active material.
_material->getTechnique(0)->getPass(_passId)->getTextureUnitState(1)->
setTexture(distortionTexture);

Copy link
Member

Choose a reason for hiding this comment

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

nit: I believe there is trailing whitespace on this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@scpeters
Copy link
Member

this fixes the issues I'm seeing in the VIPER simulation. I've even tried reverting the fixes from #3033 and #3060 with the following patch and everything still seems to work

diff --git a/gazebo/rendering/Camera.cc b/gazebo/rendering/Camera.cc
index 502f122a13..819b2a5a44 100644
--- a/gazebo/rendering/Camera.cc
+++ b/gazebo/rendering/Camera.cc
@@ -1630,7 +1630,6 @@ void Camera::SetRenderTarget(Ogre::RenderTarget *_target)
     if (this->dataPtr->distortion)
     {
       this->dataPtr->distortion->SetCamera(shared_from_this());
-      this->renderTarget->update();
     }
 
     if (this->GetScene()->GetSkyX() != NULL)
@@ -2111,15 +2110,6 @@ bool Camera::SetBackgroundColor(const ignition::math::Color &_color)
   if (this->OgreViewport())
   {
     this->OgreViewport()->setBackgroundColour(Conversions::Convert(_color));
-
-    // refresh distortion to prevent improper compositor initialization
-    // https://github.com/osrf/gazebo/pull/3033
-    if (this->dataPtr->distortion)
-    {
-      this->dataPtr->distortion->RefreshCompositor(shared_from_this());
-      this->renderTarget->update();
-    }
-
     return true;
   }
   return false;

@scpeters scpeters changed the title Workaround for wrong distortion material applied to some cameras. Fix Camera Distortion initialization: use correct material Nov 25, 2021
@scpeters
Copy link
Member

this appears to be a more complete fix for #2527

@mogumbo
Copy link
Contributor Author

mogumbo commented Nov 25, 2021

The new title "Fix Camera Distortion initialization: use correct material" is a bit misleading. My workaround actually leaves the incorrect material in place but updates its distortion texture each time a different camera renders.

@scpeters scpeters changed the title Fix Camera Distortion initialization: use correct material Workaround for wrong distortion material applied to some cameras Nov 25, 2021
@scpeters
Copy link
Member

The new title "Fix Camera Distortion initialization: use correct material" is a bit misleading. My workaround actually leaves the incorrect material in place but updates its distortion texture each time a different camera renders.

thanks; I've restored your original title

@iche033
Copy link
Contributor

iche033 commented Nov 29, 2021

ok yeah it does look like the compositor instances share the same compositor technique inside ogre. Changes look good to me.

@scpeters
Copy link
Member

homebrew CI is failing due to gazebo-tooling/release-tools#573, but otherwise the CI results look fine to me

@scpeters
Copy link
Member

rerunning macOS CI

@scpeters scpeters merged commit feff7f0 into gazebosim:gazebo11 Dec 1, 2021
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