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

From Ogre 2.1 to Ogre 2.2 #272

Merged
merged 62 commits into from
Sep 8, 2021
Merged

From Ogre 2.1 to Ogre 2.2 #272

merged 62 commits into from
Sep 8, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Mar 16, 2021

This PR updates the Ogre2 from Ogre 2.1 to Ogre 2.2.

There are some issues that need to be resolved yet

  • GPURays not working
  • When more than one rendering sensor is added the cameras are not showing anything
  • The 3DScene is completely black ( not getting the right GLID )
  • Expose EGL
  • Clean up code

Related with:

@ahcorde ahcorde added the 🏢 edifice Ignition Edifice label Mar 16, 2021
@ahcorde ahcorde self-assigned this Mar 16, 2021
@chapulina chapulina added needs upstream release Blocked by a release of an upstream library beta Targeting beta release of upcoming collection labels Mar 16, 2021
@chapulina chapulina requested a review from mjcarroll March 19, 2021 02:15
@ahcorde ahcorde force-pushed the ahcorde/update/ogre2.2 branch from f8ddf8a to ce84c10 Compare March 19, 2021 22:19
@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 19, 2021

@mjcarroll and @iche033, this PR is ready for a initial review.

There is a know issue in the Ogre2SelectionBuffer but the rest of classes should work fine:

  • Camera
  • DepthCamera
  • ThermalCamera
  • GPURay
  • LidarVisual
  • Lightvisual
  • RayQuery
  • Grid
  • Renderpass
  • Gizmo

@ahcorde ahcorde marked this pull request as ready for review March 19, 2021 23:26
@ahcorde ahcorde requested a review from iche033 as a code owner March 19, 2021 23:26
@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 22, 2021

I fixed the Ogre2Selectionbuffer.

I will create a PR on top of this one to enable EGL

@mjcarroll
Copy link
Contributor

Can you split the media updates into their own commit to make it a bit easier to review?

ahcorde added 3 commits March 23, 2021 17:57
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
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.

A few comments, moving on to checking tests and examples

if (texture == nullptr){
return;
}

// TODO(anyone) handle Bayer conversions
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be handed by Ogre::Image2 if I understand correctly?

Signed-off-by: ahcorde <[email protected]>
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

did a first pass review. Looking good

rt0TexDef->automipmaps = false;
rt0TexDef->hwGammaWrite = Ogre::TextureDefinitionBase::BoolFalse;
rt0TexDef->format = Ogre::PFG_RGBA32_FLOAT;
rt0TexDef->textureFlags |= !Ogre::TextureFlags::Uav;
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a compile warning here and other places with this line. I think it's for removing the flag? i.e. rt0TexDef->textureFlags &= ~Ogre::TextureFlags::Uav;? or maybe we can just set it to Ogre::TextureFlags::RenderToTexture

ahcorde and others added 3 commits March 24, 2021 16:07
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>

Signed-off-by: ahcorde <[email protected]>
@chapulina chapulina mentioned this pull request Mar 24, 2021
7 tasks
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #272 (95af386) into main (04b2af1) will increase coverage by 0.11%.
The diff coverage is 87.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
+ Coverage   58.59%   58.71%   +0.11%     
==========================================
  Files         174      174              
  Lines       17064    17184     +120     
==========================================
+ Hits         9999    10089      +90     
- Misses       7065     7095      +30     
Impacted Files Coverage Δ
include/ignition/rendering/Camera.hh 100.00% <ø> (ø)
include/ignition/rendering/RenderEngine.hh 100.00% <ø> (ø)
include/ignition/rendering/base/BaseCamera.hh 66.17% <0.00%> (-0.50%) ⬇️
...lude/ignition/rendering/ogre2/Ogre2RenderEngine.hh 100.00% <ø> (ø)
ogre2/src/Ogre2Conversions.cc 92.85% <0.00%> (-7.15%) ⬇️
ogre2/src/Ogre2DynamicRenderable.cc 75.22% <ø> (ø)
ogre2/src/Ogre2Geometry.cc 100.00% <ø> (ø)
ogre2/src/Ogre2ParticleNoiseListener.hh 0.00% <ø> (-100.00%) ⬇️
ogre2/src/Ogre2RenderTargetMaterial.cc 0.00% <0.00%> (ø)
ogre2/src/Ogre2Visual.cc 79.50% <ø> (ø)
... and 18 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 04b2af1...95af386. Read the comment docs.

@darksylinc
Copy link
Contributor

darksylinc commented Sep 4, 2021

INTEGRATION_lidar_visual is passing however there's lot of:

[Err] [Ogre2GpuRays.cc:262] Error casting user data: Unexpected index

in stderr which aren't happening in Edifice. I'm taking a look to see what's going on.

Update

OK it doesn't seem to be a rendering bug. This error happens before rendering.
ogreVisual->UserData(laserRetroKey) tries to retrieve the value for key laser_retro but:

  1. In Edifice everything fails until it reaches retroValue = std::get<int>(tempLaserRetro); which succeds but the value is always 0.
  2. In 2.2 branch (probably a side effect of being in Fortress, since the code involved spans several modules) everything fails, and thus the value ends up being -1. As a result, the material is never switched (because negative values are ignored) and rendering will now diverge from Edifice.

I don't see laser_retro being set anywhere except ign-gazebo/src/rendering/SceneManager.cc and ign-rendering/test/integration/gpu_rays.cc but neither of them are set during INTEGRATION_lidar_visual test.

This leads me to believe there was a change in how Variant treats undefined values by default (i.e. defaulting to int 0 vs throwing); which is unrelated to rendering or Ogre 2.1/2.2 but may have undesired consequences in the test

Probably a copy-paste bug, after enabling DEPTH_CLAMP we must disable
once we're done; but we call glEnable again instead of disabling it.

Signed-off-by: Matias N. Goldberg <[email protected]>
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
@chapulina
Copy link
Contributor

I ticketed gazebo-tooling/release-tools#509 so the macOS and Windows builds to turn yellow when Ogre 2 isn't found.

For macOS, we may need to set SKIP_ogre2 for now. But we shouldn't regress on Windows CI. What's the status of that?

@mjcarroll
Copy link
Contributor

The OGRE2 rendering plugin builds correctly on Windows with OGRE2.2, but tests fail with:

Failed to load plugin ignition-rendering-ogre2: couldn't load library on path, which is the same as with OGRE2.1, so not a regression, but not an improvement either.

@chapulina
Copy link
Contributor

Ahh I hadn't noticed that Windows found Ogre2, all good!

@iche033 , can we get this in?

@iche033
Copy link
Contributor

iche033 commented Sep 8, 2021

yes merging! 🤞

@iche033 iche033 merged commit 7def756 into main Sep 8, 2021
@iche033 iche033 deleted the ahcorde/update/ogre2.2 branch September 8, 2021 22:26
@chapulina chapulina restored the ahcorde/update/ogre2.2 branch September 8, 2021 23:28
@chapulina chapulina deleted the ahcorde/update/ogre2.2 branch September 8, 2021 23:29
adlarkin referenced this pull request Sep 16, 2021
@chapulina chapulina mentioned this pull request Oct 11, 2021
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrading for OGRE 2.2
6 participants