-
Notifications
You must be signed in to change notification settings - Fork 464
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
Fix compatiblity problems with OGRE 1.12 release #1252
Conversation
Hi, I appreciate the work you put in, and I appreciate the mention here, but the real credit is in the authorship information in the git commits. If you want to incorporate my work in your PR, you can, but then please do preserve the git history of my commits. The best option is to simply branch of from my PR. If you rebase on my branch, that should solve the issue. |
@ProfFan Yes, please preserve change attribution by forking @de-vri-es's branch if you want to build on top of his work. |
@de-vri-es @wjwwood Sorry for the confusion here :) I assumed that maintainers would first merge @de-vri-es 's PR, and then this one :) Will rebase. |
The interaction and selection tools are working now. |
Thanks for rebasing! I can probably find time for a preliminary review soon. |
@@ -105,14 +105,14 @@ void MovableText::setFontName(const String &fontName) | |||
Ogre::MaterialManager::getSingleton().remove(mName + "Material"); | |||
} | |||
|
|||
if (mFontName != fontName || mpMaterial.isNull() || !(mpFont.get())) | |||
if (mFontName != fontName || mpMaterial.isNull() || mpFont.isNull()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you swap away from !ptr.get()
? As far as I could tell, that is the only non-deprecated way of testing for nullptrs in all Ogre versions. (The parenthesis really isn't necessary though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this is to make the CI happy and to make the code look consistent. I can change all .isNull()
calls to .get()
s as .isNull()
looks really non-standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How exactly does CI get unhappy about !mpFont.get()
? I've added a similar check before without CI complaining.
@@ -8,6 +8,10 @@ if (POLICY CMP0054) | |||
cmake_policy(SET CMP0054 NEW) | |||
endif() | |||
|
|||
if (POLICY CMP0072) | |||
cmake_policy(SET CMP0072 NEW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this solve any problems, or is it just to silence warnings from CMake >= 3.11.3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think GLVND is the standard now, as it supports both mesa and Nvidia GL libs. With a if (POLICY)
statement here it should not do anything bad on older systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. Maybe a comment to explain what it does would be nice. I know the rest of the file doesn't have it, but it's probably still a good idea.
@@ -1,4 +1,4 @@ | |||
Liberation Sans | |||
font "Liberation Sans" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! The broken font had me going insane for quite a while.
@@ -69,18 +69,18 @@ void MeshResourceMarker::reset() | |||
entity_ = 0; | |||
} | |||
|
|||
|
|||
// If we use SharedPtr then there will be no need for manual deletion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the materials also unloaded from the resource manager when the last shared_ptr is destroyed?
If this works properly, you should probably remove the code entirely rather than comment it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept it there intentionally for people to facilitate reviewing. Will remove if it looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so does the Ogre documentation shed any light on what happens when the shared material pointer go out of scope? Does it indeed unload the materials from the manager, or are we still leaking them?
material_->unload(); | ||
Ogre::MaterialManager::getSingleton().remove(material_->getName()); | ||
// material_->unload(); | ||
// Ogre::MaterialManager::getSingleton().remove(material_->getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, if this works as intended, better just remove the code entirely.
@@ -441,6 +442,9 @@ void loadMaterials(const std::string& resource_path, | |||
{ | |||
std::stringstream ss; | |||
ss << resource_path << "Material" << i; | |||
|
|||
// Prevent loading a material twice | |||
if(Ogre::MaterialManager::getSingleton().getByName(ss.str()).get() != NULL) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't the resource names unique anyway? Besides, it seems unlikely that this check would ever trigger, since you're not passing the resource group name to getByName
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getByName
defaults to ResourceGroupManager::AUTODETECT_RESOURCE_GROUP_NAME
I think. This piece of code is here because in my tests Ogre panics here saying that you cannot have resources with the same name. Needs more testing though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. That's odd because in some places the default changed to DEFAULT_RESOURCE_GROUP_NAME
(see [1]), but it seems they didn't update the defaults everywhere..
Still, it would be good to know in what situation there are duplicate material names and if we can safely skip creation then.
Slight detail, but stringstream.str()
creates a copy of the internal buffer. It may be better to call it once and reuse the same copy.
return thisfont.staticCast<Ogre::Mesh>(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what situation does getByName
return false? Shouldn't this be fixed by passing the correct resource group name to getByName
instead?
The name fonts
and thisfont
also seem somewhat misleading. They're meshes, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silly me forgot to change the names when copy-pasting code. Will fix :)
Also see the previous comment.
if (!mpFont) | ||
= FontManager::getSingleton().getByName(mFontName); | ||
|
||
// Workaround for getByName bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be fixed by passing the correct resource group to getByName
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will test :)
@@ -76,6 +76,10 @@ inline uint32_t colorToHandle(Ogre::PixelFormat fmt, uint32_t col) | |||
{ | |||
handle = col >> 8; | |||
} | |||
else if (fmt == Ogre::PF_R8G8B8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add another or-clause to the first if?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is also for consistency I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the first if statement does exactly the same as this one and already has two conditions. But it doesn't matter much I suppose.
@@ -214,7 +215,7 @@ void SelectionHandler::destroyBox(const std::pair<CollObjectHandle, uint64_t>& h | |||
Ogre::WireBoundingBox* box = it->second.second; | |||
|
|||
node->detachAllObjects(); | |||
node->getParentSceneNode()->removeAndDestroyChild(node->getName()); | |||
if(node->getName().length() > 0) node->getParentSceneNode()->removeAndDestroyChild(node->getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this leak anonymous nodes?
In other places I replaced removeAndDestroyChild(node->getName())
with:
node->getParentSceneNode()->removeChild(node);
node->getCreator()->destroySceneNode(node);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. Will test.
I had a crazy idea (maybe), but since the maintainers currently don't have the time for reviewing, would it be an idea to send your PR to my branch for #1242 (https://github.com/fizyr-forks/rviz/tree/ogre1.11)? That way we can at least consolidate efforts on fixing Ogre 1.11 compatibility. Right now I need some of your fixes to continue on my branch, and if I change my branch, yours may not merge cleanly anymore. Authorship information will of course be preserved for your commits as well if we do this. |
@de-vri-es LGTM :) |
@de-vri-es Plus I think you should also rebase your branch on |
It is based on |
PR sent |
Dear @ProfFan, |
There were a few "new" commits (end of 2018) in https://github.com/ros-visualization/rviz/pull/1242/commits , could you incorporate them in this PR? I think https://github.com/ros-visualization/rviz/pull/1242/commits can be closed since this PR supersedes that PR? |
@hgaiser I think I have sent a PR to @de-vri-es 's branch and he has not responded to that one yet. Since I think you two are working in the same company can you brief us on the current situation? Many thanks! |
Ah I see. Unfortunately we don't have time to work on this issue anymore. I would suggest to try to merge this with feedback from the rviz maintainers instead. |
Now that I have finished moving I'll get this going #mark |
Because it breaks with strict resource manager mode in Ogre 1.11.
This was split off to a plugin in Ogre 1.11, and is required for decoding PNG textures.
The program compiler in Ogre 1.11 does not seem to resolve relative paths anymore.
In Ogre 1.11 deleting an anonymous node by name triggers an assertion failure.
Mesh files can't seem to specify the resource group for a material, so this is a relatively simple workaround.
The old signals library has been removed from boost 1.69. Because of that, find_package(Boost COMPONENTS signals ...) doesn't work anymore. The newer signals2 library is header only and does not need to be listed as boost component at all.
07cd9c5
to
62803f8
Compare
Rebased to latest melodic-devel. Need testers :) |
SIP processing in Catkin is a bit broken on macOS. @rhaschke What is your stance on putting in workarounds in the repo? |
If SIP is broken I would prefer a fix in the package |
By the way, many thanks for your efforts working on this PR. I hope to release rviz on this weekend and then this PR will become my top priority item for rviz. |
Is it this that broke in sip ? |
@hgaiser Similar but not exactly. |
Will now push the WIP here so you can see the problem. |
Looks like travis is broken. Ping @rhaschke can you help reviewing this PR and also tests against other targets (Flavors of Linux, etc.)? |
Closing in favor of #1434. @ProfFan, unfortunately, most of your additional commits (on top of #1242) were kind of work-arounds. I hope, I found proper solutions for the remaining issues (e.g. selection) in #1434. If you think that some of your changes are missing nonetheless, please file them in a new PR against |
This PR is based on @de-vri-es 's amazing work(PR #1242). I tested extensively on Melodic and MoveIt!, and fixed several runtime errors related to resource handling.
TODO:
Interact function is not working (at least for me on Arch Linux with GNOME 3)Fixed by 2ce1c5e[ERROR] [1528378902.538022886]: Can't assign material Triangle List Marker2Material to the ManualObject Triangle List Marker2 because this Material does not exist in group General. Have you forgotten to define it in a .material script?
errors, still need investigation.