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

Upgrade from Ogre 1.10 to Ogre 1.12.1 #394

Merged
merged 20 commits into from Aug 21, 2019
Merged

Conversation

Martin-Idel
Copy link
Contributor

@Martin-Idel Martin-Idel commented Apr 20, 2019

This PR pulls upgrades to the latest OGRE 1.x version, pulling in a number of breaking API changes.

Let's see whether this helps with #368.

This may or may not help with issues on OSX (maybe if we iterate a bit, this may help with #385 - but I don't have OSX and some fixes may be necessary to build Ogre on Mojave: OGRECave/ogre#894).

Commits are hopefully structured to make reviewing this easier.

@tfoote tfoote added the in review Waiting for review (Kanban column) label Apr 20, 2019
@Martin-Idel
Copy link
Contributor Author

Martin-Idel commented Apr 20, 2019

Sadly, this doesn't help for #368 :

  • Linux-aarch64 Build Status

I can start a full CI if you'd like to perform the upgrade.

@Martin-Idel Martin-Idel changed the title Upgrade from Ogre 1.10 to Ogre 1.11.5 Upgrade from Ogre 1.10 to Ogre 1.12.0 May 5, 2019
@VictorLamoine
Copy link
Contributor

I know I'm late to the party but why don't we use Ogre 2?

@Martin-Idel
Copy link
Contributor Author

@VictorLamoine

Ogre 2 uses a different API - in part its vastly different. This effort was deemed too much in the beginning of the migration and it would probably (?) change the RViz plugins API and require remigration of plugins. The benefits are mostly only in terms of performance on modern architectures.

Since Ogre 1 is (for now) still maintained and actively developed (in part because it is used by gazebo and RViz), we can stay on Ogre 1.

However, out of curiosity I'm investigating what it would actually mean to upgrade to Ogre 2 and I hope to be able to show this soonish.

Note also that rviz_rendering was originally designed to encapsulate all Ogre usage to be able to swap out the implementation at some point. It turned out that Ogre is (as of now) used in too many aspects throughout the code base - especially in the plugin API so that this effort was not pursued further.

@mjcarroll
Copy link
Member

@iche033 could probably provide some more input on what changes may be needed, as he has been going through this with ign-rendering, for use with the next-generation gazebo.

I know that the shaders were an issue, and had to be modified for OGRE2.

@iche033
Copy link

iche033 commented Jul 9, 2019

From the top of my head, here are the changes I had to do when porting to use ogre 2.x for ign-rendering

  • upgrade old ogre 1.x materials to the ogre 2.x's HLMS.
    • the old materials still exist - at least on the API level but I found that some functionalities are no longer available so I upgraded them all to HLMS.
  • upgrading glsl shaders to version >150
  • update engine initialization and resource setup
    • this is more straightforward.
  • convert cameras to use compositors
  • fix various API breakages

Other changes:

  • I had to change the way selection buffer (for mouse picking) is implemented since material schemes are not supported anymore.
  • We're using ogre RTSS in gazebo and that's no longer available so another reason that I updated the materials to use the PBS component of HLMS.
  • I spend quite a bit of time integrating ogre2.x with QML as it's not as trivial as integration with QWidgets. Yet there are still issues on macOS (and very likely on Windows too).

I'm not very familiar with rviz code base but a quick look shows that things like movable texts, and point clouds may also need to be updated since it's using the old material system.

@Martin-Idel Martin-Idel changed the title Upgrade from Ogre 1.10 to Ogre 1.12.0 Upgrade from Ogre 1.10 to Ogre 1.12.1 Jul 26, 2019
@Martin-Idel
Copy link
Contributor Author

Thanks for the information. That was about what I expected and then some :-D.

Anyway, until then I rebased this PR and updated it to the latest release (no changes).

@Martin-Idel
Copy link
Contributor Author

@wjwwood , @jacobperron
As far as I can see, the latest changes to 1.12.1 (or maybe even to 1.12?) actually make the tests pass on aarch in repeated mode:

  • Linux-aarch64 Build Status

So this PR seems to finally fix #368 - I know that tests are currently disabled in the nightlies so they could be reenabled once this is merged.

@Martin-Idel Martin-Idel force-pushed the update_ogre branch 2 times, most recently from 0fa4914 to b4e8bda Compare August 5, 2019 14:00
- old boost patch seems unnecessary
- Could not be loaded anymore
- upstream Ogre now uses Find modules that CMake provides
- However, this resulted in freetype not being found
The underlying data structure changed so we don't
need the reinterpret casts anymore.
Leaving the static_casts to make explicit what we
expect at that point and force action if the model
changes again on some update
@Martin-Idel
Copy link
Contributor Author

I rebased the branch again and added commits starting with one commit fixing the Ogre::Overlay build on Windows for me.

@wjwwood
Copy link
Member

wjwwood commented Aug 5, 2019

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

I'll consider re enabling the tests that are disabled on nightly if these pass.

@Martin-Idel
Copy link
Contributor Author

So it's "just" a few thousand Windows warnings and otherwise it works. My first try to get rid of them seems to not work on my machine, so I'll have to think of a true fix probably...

@Martin-Idel
Copy link
Contributor Author

It's just one warning in many iterations but as far as I can see, there is no true fix. However, the code is completely safe as the problem the warning C4251 describes cannot occur: we compile both Ogre and RViz with the same compiler.

The only way to get rid of the warnings would be to disable them. There are three ways, which would you prefer?

  • disable the warning via CMake (I'd prefer this, but couldn't get it to work, just adding add_compile_options(/wd4251) for MSVC-compiler didn't do the trick)
  • add pragmas to virtually all OGRE headers via a patch
  • add pragmas around many Ogre includes in RViz files

Here is why I think the warning cannot be fixed:

The new templated Ogre::Vector class instantiates a few members (such as Vector3::ZERO, etc.) inside the library while the template definition is also provided and exposed in the header file. This means that when I use e.g. Ogre::Vector3 in RViz, I will generate my own vector instantiation while I use the one in Ogre's library when I call Ogre::Vector3::ZERO. That's exactly the situation which generates the C4251 warning and what it's for.

I thought one could maybe explicitly instantiate all vectors needed for RViz within Ogre, but that is not possible because the Ogre code relies on implicit (lazy) template instantiation - for instance, it defines a three argument constructor for all vectors, but then provides a specialization for 2-dim vectors which doesn't have a three argument constructor so that when I explicitly instantiate a 2-dim vector, I get an error about the missing three argument constructor...

@Martin-Idel Martin-Idel force-pushed the update_ogre branch 3 times, most recently from 5e087ef to 34da950 Compare August 11, 2019 19:29
@Martin-Idel
Copy link
Contributor Author

@wjwwood I somehow couldn't disable the warning C4251 via CMake, something seems to override the settings. If you have a better idea, I'm happy to try that.

For now, I added pragmas to Ogre via a patch (making this PR quite large, but most of it is a diff-file) using sed scripts. As described above, I don't see a way to actually fix the warnings. The patch is only applied on Windows.

Since I was on the task, I also cleaned up a bit: All old Ogre pragmas for other warnings are gone since they are no longer necessary and I also changed the project name of the ogre ExternalProject.

I took the liberty to redo CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood
Copy link
Member

wjwwood commented Aug 21, 2019

I'm ok with the patch approach. Thanks for looking into it, I know disabling those warnings is a PITA when it's on Windows and an external project.

@wjwwood wjwwood merged commit eabf596 into ros2:ros2 Aug 21, 2019
@bpwilcox bpwilcox mentioned this pull request Aug 26, 2019
clalancette added a commit to ros2/ci that referenced this pull request May 3, 2021
This should have been fixed long ago by
ros2/rviz#394.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette added a commit to ros2/ci that referenced this pull request May 24, 2021
This should have been fixed long ago by
ros2/rviz#394.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants