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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Support for wide-angle cameras in ogre2 #733

Merged
merged 8 commits into from Oct 31, 2022

Conversation

darksylinc
Copy link
Contributor

@darksylinc darksylinc commented Oct 9, 2022

馃帀 New feature

Closes #729

Summary

This PR ports OgreWideAngleCamera to Ogre2 engine.

Includes Vulkan, OpenGL & Metal shaders (Metal support not tested)

Test it

Edit wide_angle_camera.sdf so that:

    <plugin
      filename="gz-sim-sensors-system"
      name="gz::sim::systems::Sensors">
      <render_engine>ogre2</render_engine>
    </plugin>

No longer says ogre and run:

gz sim wide_angle_camera.sdf

Can it be backported to Garden?

The only major/blocking issue I see is that Ogre2Scene::CreateWideAngleCameraImpl was added (and is obviously necessary) but adding it breaks the ABI.

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.

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Merging #733 (372a256) into main (fcd52be) will increase coverage by 0.33%.
The diff coverage is 90.45%.

@@            Coverage Diff             @@
##             main     #733      +/-   ##
==========================================
+ Coverage   74.07%   74.40%   +0.33%     
==========================================
  Files         159      160       +1     
  Lines       14408    14684     +276     
==========================================
+ Hits        10673    10926     +253     
- Misses       3735     3758      +23     
Impacted Files Coverage 螖
ogre2/src/Ogre2WideAngleCamera.cc 90.00% <90.00%> (酶)
ogre2/src/Ogre2RenderTarget.cc 82.16% <100.00%> (+0.98%) 猬嗭笍
ogre2/src/Ogre2Scene.cc 78.33% <100.00%> (+0.13%) 猬嗭笍

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mjcarroll mjcarroll moved this from Inbox to In review in Core development Oct 13, 2022
@mjcarroll mjcarroll assigned mjcarroll and darksylinc and unassigned mjcarroll Oct 13, 2022
@iche033
Copy link
Contributor

iche033 commented Oct 13, 2022

I enabled the INTEGRATION_wide_angle_camera test for ogre2 with this patch:

diff --git a/test/integration/wide_angle_camera.cc b/test/integration/wide_angle_camera.cc
index 31481a72..2b604376 100644
--- a/test/integration/wide_angle_camera.cc
+++ b/test/integration/wide_angle_camera.cc
@@ -68,7 +68,7 @@ void OnNewWideAngleFrame(const unsigned char *_data,
 //////////////////////////////////////////////////
 TEST_F(WideAngleCameraTest, GZ_UTILS_TEST_DISABLED_ON_WIN32(WideAngleCamera))
 {
-  CHECK_SUPPORTED_ENGINE("ogre");
+  CHECK_UNSUPPORTED_ENGINE("optix");
 
   gz::rendering::ScenePtr scene = engine->CreateScene("scene");
   ASSERT_NE(nullptr, scene);
@@ -205,7 +205,7 @@ TEST_F(WideAngleCameraTest, GZ_UTILS_TEST_DISABLED_ON_WIN32(WideAngleCamera))
 //////////////////////////////////////////////////
 TEST_F(WideAngleCameraTest, GZ_UTILS_TEST_DISABLED_ON_WIN32(Projection))
 {
-  CHECK_SUPPORTED_ENGINE("ogre");
+  CHECK_UNSUPPORTED_ENGINE("optix");
 
   gz::rendering::ScenePtr scene = engine->CreateScene("scene");
   ASSERT_NE(nullptr, scene);

The first test passed but a few checks are failing in the Projection test

@darksylinc
Copy link
Contributor Author

Perfect! I'll take a look after-tomorrow at those test failures.

@darksylinc
Copy link
Contributor Author

I've enabled the Ogre2WideAngleCamera test for ogre2 and fixed its failures. It's now passing.

@darksylinc
Copy link
Contributor Author

Nevermind, ignition_rendering-ci-pr_any-ubuntu_auto-amd64 CI says INTEGRATION_wide_angle_camera_ogre2_gl3plus fails with:

/home/jenkins/workspace/ignition_rendering-ci-pr_any-ubuntu_auto-amd64/ign-rendering/test/integration/wide_angle_camera.cc:252: Failure
57: Expected: (screenPt.Z()) > (1.0), actual: 8.84479e-08 vs 1

I can repro it. Looking into it.

@darksylinc
Copy link
Contributor Author

darksylinc commented Oct 15, 2022

Silly last-minute bug had crawled into the code; that's why the test failed. It should be working now.

Edit: I see the CI tests Metal as well! I managed to find a clever way to test whether the Metal shaders were valid without having to compile gazebo on a Mac; so I fixed all the shader compiler errors. Now let's cross fingers the CI passes on Metal too!

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Just a few notes on a quick review. Will give more feedback once I have a chance to run it.

ogre2/include/gz/rendering/ogre2/Ogre2Scene.hh Outdated Show resolved Hide resolved
ogre2/src/Ogre2WideAngleCamera.cc Show resolved Hide resolved
ogre2/src/Ogre2WideAngleCamera.cc Outdated Show resolved Hide resolved
@mjcarroll
Copy link
Contributor

mjcarroll commented Oct 26, 2022

This doesn't have anything to do with your changes (unless something is really wrong), but it popped here:

No action required, just posting for posterity, because it probably needs to be addressed.

69: Test command: C:\Jenkins\workspace\ign_rendering-pr-win\ws\build\gz-rendering8\bin\Release\UNIT_Material_TEST.exe "--gtest_output=xml:C:/Jenkins/workspace/ign_rendering-pr-win/ws/build/gz-rendering8/test_results/UNIT_Material_TEST_ogre_gl3plus.xml"
69: Environment variables: 
69:  GZ_ENGINE_TO_TEST=ogre
69:  GZ_ENGINE_BACKEND=gl3plus
69: Test timeout computed to be: 240
69: Running main() from C:\Jenkins\workspace\ign_rendering-pr-win\ws\gz-rendering\test\gtest_vendor\src\gtest_main.cc
69: [==========] Running 2 tests from 1 test suite.
69: [----------] Global test environment set-up.
69: [----------] 2 tests from MaterialTest
69: [ RUN      ] MaterialTest.MaterialProperties
69: 锟絒32m[Msg] 锟絒m锟絒32mLoading plugin [锟絒m锟絒32mgz-rendering-ogre锟絒m锟絒32m]锟絒m锟絒32m
69: 锟絒m[       OK ] MaterialTest.MaterialProperties (651 ms)
69: [ RUN      ] MaterialTest.Copy
69: 锟絒32m[Msg] 锟絒m锟絒32mLoading plugin [锟絒m锟絒32mgz-rendering-ogre锟絒m锟絒32m]锟絒m锟絒32m
69: 锟絒munknown file: error: C++ exception with description "Ogre::ItemIdentityException::ItemIdentityException: Texture with the name C:\Jenkins\workspace\ign_rendering-pr-win\ws\gz-rendering\test\media\materials\textures\texture.png already exists. in ResourceManager::add at C:\vcpkg\buildtrees\ogre\src\eddf310f0b-6ab1152694.clean\OgreMain\src\OgreResourceManager.cpp (line 149)" thrown in the test body.
69: [  FAILED  ] MaterialTest.Copy (429 ms)
69: [----------] 2 tests from MaterialTest (1080 ms total)
69: 
69: [----------] Global test environment tear-down
69: [==========] 2 tests from 1 test suite ran. (1081 ms total)
69: [  PASSED  ] 1 test.
69: [  FAILED  ] 1 test, listed below:
69: [  FAILED  ] MaterialTest.Copy
69: 
69:  1 FAILED TEST
 69/110 Test  #69: UNIT_Material_TEST_ogre_gl3plus ......................***Failed    1.40 sec
test 70
        Start  70: check_UNIT_Material_TEST_ogre_gl3plus

Ogre2WideAngleCamera is working!

Add missing MSAA & Shadows to Ogre2WideAngleCamera

Add Metal shaders to Ogre2WideAngleCamera

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Fix Ogre2WideAngleCamera::Project3d which was failing tests

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Ogre::Camera::getProjectionMatrix returns a matrix with Z [-1; 1]
without reverse Z (regardless of reverse Z setting).

We must check z is in range [-1; 1] and not just pos.z >= -1 because
otherwise pos.z = 1.02 is a possible outcome and gets picked by the
wrong face; thus failing the tests.

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
@mjcarroll mjcarroll merged commit c690cec into gazebosim:main Oct 31, 2022
Core development automation moved this from In review to Done Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
馃幍 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants