Inconsistent FOV & Aspect Ratio handling in ogre2

[darksylinc]

Environment

• OS Version: Xubuntu 20.04
• Source or binary build?
  Garden, probably others too.
• If this is a GUI or sensor rendering bug, describe your GPU and rendering system. Otherwise delete this section.
  • Rendering plugin: [ogre2]

Description

While creating Heightmap integration tests, I noticed Camera and DepthCamera produced quite different results; despite having the same parameters.

Tracking down the root of the problem, I nailed down on how gz-rendering handles Aspect Ratio inconsistently and very awkwardly.

Here are the problems:

Most Gazebo cameras call Ogre::Camera::setAutoAspectRatio( true )

This looks undesirable because Gazebo has no toggle for auto aspect ratio; but instead asks for an explicit AspectRatio

When this is set; Ogre will overwrite calls to Camera::setAspectRatio during rendering; which means gz::Ogre2Camera::AspectRatio won't be honored (assuming the AR is different from the requested resolution)

Some form of gz::Ogre2Camera::AspectRatio is still "honored" in some form because Gazebo asks for HFOV while Ogre asks for VFOV. And for the conversion the AR is still needed. This means Gazebo does:

vfov = 2.0 * atan(tan(this->HFOV().Radian() / 2.0) / this->AspectRatio());
this->ogreCamera->setFOVy(Ogre::Radian(this->LimitFOV(vfov)));

Hence calls to gz::Camera::SetAspectRatio will appear to make an impact, but it may be producing the wrong output.

Ogre2Camera::AspectRatio fallbacks to Ogre::Camera::getAspectRatio

Because of the setAutoAspectRatio() call, the following will fail:

gzCamera->SetImageWidth( 1600 );
gzCamera->SetImageHeight( 900 );
gzCamera->SetHFOV( 1 );

gzCamera->Render();

assert( gzCamera->HFOV() == 1 ); // FAIL: Ogre modified the FOV due to setAutoAspectRatio.

An integration test can be made for this due to its simplicity.

SetHFOV depends on AspectRatio

This means with the current API order matters and the following code is wrong:

camera->SetHFOV( value );       // Error AspectRatio is uninitialized
camera->SetAspectRatio( ar );

And the following code is correct:

camera->SetHFOV( value );
camera->SetAspectRatio( ar );

The most user-friendly solution would be to set the HFOV to Ogre every time in PreRender; this way initialization order won't matter.

Ogre2DepthCamera::HFOV does not rely on Ogre::Camera

This is correct IMO, but inconsistent with Ogre2Camera.

Ogre2DepthCamera only ever sets HFOV in CreateDepthTexture

Again calling order matters.

This fails:

depthCamera->CreateDepthTexture();
depthCamera->SetHFOV( value );       // Value will be ignored

This succeeds:

depthCamera->SetHFOV( value );
depthCamera->CreateDepthTexture();

• Expected behavior:
  • All cameras should be consistent and produce the same results.
  • Calling order should not matter. If it does, then Gazebo should produce an error and complain very loudly.
  • Ogre should not override Aspect Ratio. The value set by the user should be honored.
• Actual behavior:
  • Ogre2Camera and Ogre2DepthCamera may produce different output
  • Calling order matters. Using the wrong order will cause some parameters to be ignored, overriden, or result in wrong internal calculations.
  • Arbitrary AR set by user is being ignored

Steps to reproduce

Output

Output by normal Camera:

Output by normal DepthCamera:

Fix

I will be fixing this issue and submit a PR for it. So just assign this ticket to me.

[darksylinc]

OK I'm hitting a few problems when trying to fix this issue in an ABI stable way because:

• Fixing this issue means disabling AutoAspectRatio to respect the one set by caller
• However if caller always left the default value (1.3333) then rendering output gets visibly messed up
• test/common_test/Camera_TEST.cc fails for this very own reason. It sets width, height and HFOV, but not Aspect Ratio.

Technically I could fix Camera_TEST and push a PR that is ABI stable.

But while it would pass all the checks, it feels like morally I'm breaking the ABI due to Hyrum's Law:

With a sufficient number of users of an API,
it does not matter what you promise in the contract:
all observable behaviors of your system
will be depended on by somebody.

So I began to question what is Aspect Ratio and why we need it?

Aspect Ratio at low level makes sense

At OgreNext level (or anything lower level), AR makes sense. Cameras can be reused for multiple RenderTargets of varying resolutions. They may even be used to draw to only a portion of that target.

In OgreNext we added setAutoAspectRatio because most users don't care and want the AR to match that of the Viewport.

Aspect Ratio at Gazebo level doesn't make much sense

Gazebo seems to map 1 Camera == 1 physical camera in the real world. It's got its own feeds, its own render target resolution.

Hence the AR should be tied to its RenderTarget resolution.

But we have to admit, there's always "that one user", that needs a custom AR.

Solution?

As far as I can see, one approach I can try is to pretend to use Auto Aspect Ratio unless SetAspectRatio is explicitly called.

This should fix it in the most sensible way without breaking everything for everyone.

[iche033]

As far as I can see, one approach I can try is to pretend to use Auto Aspect Ratio unless SetAspectRatio is explicitly called.

agreed

[darksylinc]

agreed

The final solution I arrived turned out to be more elegant and simple than I could imagine: When you try to set an invalid value (i.e. AR <= 0.0) Gazebo starts autocalculating and Camera::AspectRatio will return the autocalculated ratio instead of 0.0 or the invalid negative value it was set.

Thus if a user explicitly calls SetAspectRatio( 2.0 ) that value will be respected.
And if the user never calls the function, the (new) default value of 0.0 will cause Gazebo to return the auto calculated value.

The only case where my PR will break user code is if the user explicitly relied on calling SetAspectRatio with an invalid value (e.g. 0.0 or negative) which would have been a very, very bad practice.