Change aspect to AspectRatio() #635

AzulRadio · 2022-05-26T20:01:37Z

🦟 Bug fix

Fixes ignition crash bug introduced in gz-sim#1499

Summary

gz-sim#1499 adds a feature to change the FOV of the user camera at runtime. But if we scale the window after changing the FOV, Gazebo has a chance to crash. (Always crash on Ogre, sometimes crash on Ogre2)

Changing aspect to AspectRatio() in OgreCamera::SetHFOV() solves the bug.

Also add comment to warn users not to use aspect in BaseCamera but to use OrgeCamera::AspectRatio() instead.

Why this bug happened:

Resizing the window needs the correct aspect ratio value. There are 2 aspect ratio values available: one in Ogre library (AspectRatio()), and the other is in BaseCamera (this->aspect).

When we change the FOV and resize the window, the aspect ratio and fov value in the Ogre library will both change but aspect in the BaseCamera only update its fov value but not aspect ratio. However, in Ogre2Camera::SetHFOV(), it is still using the deprecated aspect ratio value from the BaseCamera.

If you check with gdb, the value read by AspectRatio() (directly from Ogre) and aspect from BaseCamera no longer agree with each other after changing FOV and resizing the window. That is why there is a crash.

Checklist

Signed all commits for DCO
Added tests
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.

Signed-off-by: youhy <[email protected]>

ahcorde

linters https://github.com/gazebosim/gz-rendering/runs/6615769097?check_suite_focus=true

jennuine

Other than linters this LGTM, will need @iche033 's stamp of approval

include/ignition/rendering/base/BaseCamera.hh

iche033 · 2022-05-26T21:19:26Z

Ideally we also update the value of this->aspect after setting FOV and/or image size, potentially in BaseCamera's SetImageWidth / SetImageHeight functions so that it's not outdated. Can you also add a quick test in Camera_TEST.cc?

Signed-off-by: youhy <[email protected]>

codecov · 2022-05-26T22:09:49Z

Codecov Report

Merging #635 (1c581e9) into ign-rendering6 (a3dc1ae) will decrease coverage by 25.47%.
The diff coverage is 87.50%.

❗ Current head 1c581e9 differs from pull request most recent head 545a534. Consider uploading reports for the commit 545a534 to get more accurate results

@@                 Coverage Diff                 @@
##           ign-rendering6     #635       +/-   ##
===================================================
- Coverage           80.00%   54.52%   -25.48%     
===================================================
  Files                   1      198      +197     
  Lines                  15    20270    +20255     
===================================================
+ Hits                   12    11052    +11040     
- Misses                  3     9218     +9215

Impacted Files	Coverage Δ
ogre/src/OgreCamera.cc	`0.00% <0.00%> (ø)`
include/ignition/rendering/base/BaseCamera.hh	`66.66% <100.00%> (ø)`
ogre2/src/Ogre2Camera.cc	`88.88% <100.00%> (ø)`
ogre2/src/Ogre2DepthCamera.cc	`88.37% <100.00%> (ø)`
ogre2/src/Ogre2ThermalCamera.cc	`88.60% <100.00%> (ø)`
test_config.h
ogre2/src/Ogre2RenderTarget.cc	`86.63% <0.00%> (ø)`
ogre2/src/Ogre2LidarVisual.cc	`80.95% <0.00%> (ø)`
ogre2/src/Ogre2Light.cc	`98.30% <0.00%> (ø)`
ogre/src/OgreRenderPass.cc	`0.00% <0.00%> (ø)`
... and 194 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3dc1ae...545a534. Read the comment docs.

AzulRadio · 2022-05-27T15:30:19Z

@iche033 Just to clarify, do you mean we should update this->aspect in BaseCamera::SetImageWidth, BaseCamera::SetImageHeight, and BaseCamera::SetHFOV, and add a test to check if aspect agrees with AspectRatio?

iche033 · 2022-05-31T20:22:34Z

BaseCamera::SetImageWidth, BaseCamera::SetImageHeight

yes I would expect the aspect ratio to change when user calls these 2 functions. I think the HFOV should not affect aspect ratio though but we should double check that.

Signed-off-by: youhy <[email protected]>

AzulRadio · 2022-05-31T22:28:59Z

@iche033 Aspect ratio change added in fbbbc4b. I'm not sure how to add the test to Camera_TEST because aspect is a protected variable (not accessible in Camera_TEST). I verified with GDB and it's now changing when resizing the window.

jennuine

For a Camera_TEST you can check that after SetImageWidth AspectRatio() agrees.

e.g.,

double width = 100
camera.SetImageWidth(width)
double aspectRatio = width / camera.ImageHeight()
EXPECT_NEAR(aspectRatio, camera.AspectRatio(), 1e-6)

Similarly for SetImageHeight

ogre2/src/Ogre2Camera.cc

Signed-off-by: youhy <[email protected]>

Linters fixed

Signed-off-by: youhy <[email protected]>

jennuine · 2022-06-06T17:30:53Z

src/Camera_TEST.cc

+  double width = 100
+  camera.SetImageWidth(width)
+  double aspectRatio = width / camera.ImageHeight()
+  EXPECT_NEAR(aspectRatio, camera.AspectRatio(), 1e-6)


This is missing ; at the end of each line and camera is a pointer. This test fails though, you will need to try to investigate why and how to correct this

Since you are using ogre on your system, temporarily update ogre2 to ogre here:

gz-rendering/test/test_config.h.in

Line 16 in a3dc1ae

static const std::vector<const char *> kRenderEngineTestValues{"ogre2", "optix"};

To compile the test, run colcon build --cmake-args -DBUILD_TESTING=ON --merge-install --packages-select ignition-rendering6 in the root of your workspace. To run the test you can do ./build/ignition-rendering6/bin/UNIT_Camera_Test

You should also add a similar test with SetImageHeight

Added in 8d4e542

Signed-off-by: youhy <[email protected]>

jennuine

Nice work! Left some minor comments

jennuine · 2022-06-08T17:34:51Z

include/ignition/rendering/base/BaseCamera.hh

@@ -286,6 +286,7 @@ namespace ignition
    void BaseCamera<T>::SetImageWidth(const unsigned int _width)
    {
      this->RenderTarget()->SetWidth(_width);
+      this->SetAspectRatio(1.0 * _width / this->ImageHeight());


I assume you are multiplying by 1.0 to cast the result to a double. It's better to use static_cast<double> to be explicit with the cast making the purpose clear. Same for SetImageHeight

Suggested change

this->SetAspectRatio(1.0 * _width / this->ImageHeight());

this->SetAspectRatio(

static_cast<double>(_width) / static_cast<double>(this->ImageHeight()));

jennuine · 2022-06-08T17:54:24Z

src/Camera_TEST.cc

  camera->SetImageHeight(80u);
  EXPECT_EQ(80u, camera->ImageHeight());

+  double height = 80;
+  camera->SetImageHeight(height);
+  double aspectRatio = camera->ImageWidth() / height;
+  EXPECT_NEAR(aspectRatio, camera->AspectRatio(), 1e-6);


Since your new tests are almost the same as the previous test, lets remove the new test and continue on the previous one. E.g.,

Suggested change

camera->SetImageHeight(80u);

EXPECT_EQ(80u, camera->ImageHeight());

double height = 80;

camera->SetImageHeight(height);

double aspectRatio = camera->ImageWidth() / height;

EXPECT_NEAR(aspectRatio, camera->AspectRatio(), 1e-6);

unsigned int height = 80;

camera->SetImageHeight(height);

EXPECT_EQ(height, camera->ImageHeight());

double aspectRatio =

static_cast<double>(camera->ImageWidth()) / static_cast<double>(height));

EXPECT_NEAR(aspectRatio, camera->AspectRatio(), 1e-6);

The same should be done for width.

Also, go ahead and update this expectation below to use the new variables:

gz-rendering/src/Camera_TEST.cc

Line 195 in 8d4e542

EXPECT_EQ(100u*80u*3u, camera->ImageMemorySize());

e.g., EXPECT_EQ(width*height*3u, ...)

Signed-off-by: youhy <[email protected]>

jennuine

LGTM, @iche033 do you mind to take another look?

iche033

looks good to me.

AzulRadio added 3 commits May 26, 2022 11:26

change aspect to AspectRatio()

df5d6d9

Signed-off-by: youhy <[email protected]>

thermal and depth cam aspect ratio, didnt test

a7cdf54

Signed-off-by: youhy <[email protected]>

add comment to aspect in BaseCamera

9723e19

Signed-off-by: youhy <[email protected]>

AzulRadio requested review from iche033 and WilliamLewww as code owners May 26, 2022 20:01

github-actions bot added the 🏯 fortress Ignition Fortress label May 26, 2022

ahcorde previously requested changes May 26, 2022

View reviewed changes

jennuine reviewed May 26, 2022

View reviewed changes

include/ignition/rendering/base/BaseCamera.hh Outdated Show resolved Hide resolved

AzulRadio added 2 commits May 26, 2022 14:32

update aspect in SetHFOV

2286b60

Signed-off-by: youhy <[email protected]>

codecheck fix

8133de4

Signed-off-by: youhy <[email protected]>

chapulina added the OOBE 📦✨ Out-of-box experience label May 27, 2022

update aspect ratio after resizing

fbbbc4b

Signed-off-by: youhy <[email protected]>

jennuine reviewed Jun 1, 2022

View reviewed changes

ogre2/src/Ogre2Camera.cc Show resolved Hide resolved

remove change of aspect after changing HFOV

932053a

Signed-off-by: youhy <[email protected]>

AzulRadio added 2 commits June 3, 2022 14:08

add aspect ratio for camera test

dcd5983

Signed-off-by: youhy <[email protected]>

Merge branch 'ign-rendering6' into azulradio/HFOV_fix

666638b

Signed-off-by: youhy <[email protected]>

jennuine reviewed Jun 6, 2022

View reviewed changes

AzulRadio added 2 commits June 6, 2022 13:19

fix camera test

2ad35e3

Signed-off-by: youhy <[email protected]>

add SetAspectRatio to SetImageHeight and SetImageWidth

8d4e542

Signed-off-by: youhy <[email protected]>

jennuine reviewed Jun 8, 2022

View reviewed changes

static cast to double

545a534

Signed-off-by: youhy <[email protected]>

jennuine reviewed Jun 9, 2022

View reviewed changes

iche033 approved these changes Jun 9, 2022

View reviewed changes

AzulRadio merged commit da11d8e into ign-rendering6 Jun 9, 2022

AzulRadio deleted the azulradio/HFOV_fix branch June 9, 2022 18:15

chapulina mentioned this pull request Aug 4, 2022

Merge ign-gui6 ➡️ main gazebosim/gz-gui#446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change aspect to AspectRatio() #635

Change aspect to AspectRatio() #635

AzulRadio commented May 26, 2022 •

edited by jennuine

Loading

ahcorde left a comment

jennuine left a comment

iche033 commented May 26, 2022

codecov bot commented May 26, 2022 •

edited

Loading

AzulRadio commented May 27, 2022

iche033 commented May 31, 2022

AzulRadio commented May 31, 2022

jennuine left a comment

jennuine Jun 6, 2022 •

edited

Loading

jennuine Jun 6, 2022

AzulRadio Jun 7, 2022

jennuine left a comment

jennuine Jun 8, 2022

jennuine Jun 8, 2022

jennuine left a comment

iche033 left a comment

	this->SetAspectRatio(1.0 * _width / this->ImageHeight());
	this->SetAspectRatio(
	static_cast<double>(_width) / static_cast<double>(this->ImageHeight()));

Change aspect to AspectRatio() #635

Change aspect to AspectRatio() #635

Conversation

AzulRadio commented May 26, 2022 • edited by jennuine Loading

🦟 Bug fix

Summary

Why this bug happened:

Checklist

ahcorde left a comment

Choose a reason for hiding this comment

jennuine left a comment

Choose a reason for hiding this comment

iche033 commented May 26, 2022

codecov bot commented May 26, 2022 • edited Loading

Codecov Report

AzulRadio commented May 27, 2022

iche033 commented May 31, 2022

AzulRadio commented May 31, 2022

jennuine left a comment

Choose a reason for hiding this comment

jennuine Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

jennuine Jun 6, 2022

Choose a reason for hiding this comment

AzulRadio Jun 7, 2022

Choose a reason for hiding this comment

jennuine left a comment

Choose a reason for hiding this comment

jennuine Jun 8, 2022

Choose a reason for hiding this comment

jennuine Jun 8, 2022

Choose a reason for hiding this comment

jennuine left a comment

Choose a reason for hiding this comment

iche033 left a comment

Choose a reason for hiding this comment

AzulRadio commented May 26, 2022 •

edited by jennuine

Loading

codecov bot commented May 26, 2022 •

edited

Loading

jennuine Jun 6, 2022 •

edited

Loading