-
Notifications
You must be signed in to change notification settings - Fork 212
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
Conversation
Sadly, this doesn't help for #368 : I can start a full CI if you'd like to perform the upgrade. |
I know I'm late to the party but why don't we use Ogre 2? |
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 |
@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. |
From the top of my head, here are the changes I had to do when porting to use ogre 2.x for ign-rendering
Other changes:
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. |
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). |
@wjwwood , @jacobperron 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. |
0fa4914
to
b4e8bda
Compare
- 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
b4e8bda
to
b61bdea
Compare
I rebased the branch again and added commits starting with one commit fixing the |
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... |
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?
Here is why I think the warning cannot be fixed: The new templated 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... |
5e087ef
to
34da950
Compare
34da950
to
dcf7e18
Compare
@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 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 I took the liberty to redo CI: |
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. |
This should have been fixed long ago by ros2/rviz#394. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This should have been fixed long ago by ros2/rviz#394. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
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.