Fix Windows build #175
Fix Windows build #175
Conversation
- needs to export some more symbols from rviz_common - small uncrustify changes
Don't link against Plugins anymore (loaded dynamically) Set ogre_plugin_dir to load dynamically Release and Debug need different behaviour
- this is now consistent with rviz_common and rviz_default_plugins - fixup location of ogre_testing_environment - ogre_testing_environment needs to be exported
- need to expose symbols of billboard line for Windows - copy constructor of billboard line is not generated on Windows
- just like rviz2.exe, tests need dll directories in %PATH% - add local install to environment variables for testing - in isolated builds, ogre resources cannot be found otherwise - enable tests in rviz_default_plugins on Windows but disable display
- this does not work with RVIZ_DEFAULT_PLUGINS_PUBLIC - delete tests for templated function - hide templated function again
|
I pushed two new commits which, along with ros/resource_retriever#14, allow rviz to run. However, ogre crashes on my VM, which I believe is due to not having a high enough opengl version... Ogre 1.10 seems to have really taken a step backwards in support for older opengl versions. |
|
That seems bad. However, if I read it correctly, it should support older OpenGL versions: http://wiki.ogre3d.org/Hardware, maybe it's an Intel problem? |
|
@Martin-Idel-SI no, not an Intel problem, this is in VirtualBox on my nvidia based desktop. |
|
I started a Windows CI just to see where we're at: |
| @@ -47,6 +47,7 @@ | |||
| # include "rviz_common/properties/float_property.hpp" | |||
| # include "rviz_common/properties/int_property.hpp" | |||
| # include "rviz_common/properties/queue_size_property.hpp" | |||
| #include "../../visibility_control.hpp" | |||
There was a problem hiding this comment.
This relative import does not work when these files are automoc'ed on Windows, e.g.:
03:48:15 c:\j\workspace\ci_windows\ws\build\rviz_default_plugins\rviz_default_plugins_autogen\wgdjm44luk\../../../../src/ros2/rviz/rviz_default_plugins/src/rviz_default_plugins/displays/image/image_display.hpp(50): fatal error C1083: Cannot open include file: '../../visibility_control.hpp': No such file or directory (compiling source file C:\J\workspace\ci_windows\ws\build\rviz_default_plugins\rviz_default_plugins_autogen\mocs_compilation.cpp) [C:\J\workspace\ci_windows\ws\build\rviz_default_plugins\rviz_default_plugins.vcxproj]
I recommend making that header public instead...
Actually, what is the issue you run into when you don't use visibility macros? I would have expected the plugins to not need them...
There was a problem hiding this comment.
I think it actually works fine, but the visibility_control.hpp file is currently missing from the branch, since it's not my code. Once you add it to the specified location it should work - at least it worked locally.
We need public headers because of the tests. They are linked against the library.
If you want, we can make a public include folder and get rid of all relative paths.
Why does that matter? This file came up in the initial review of rviz's code base and we determined it was actually public domain code from here: http://www.nedprod.com/programs/gccvisibility.html
No that makes sense, however, we could instead just add the source files to the tests, rather than linking it to the library or we could build a static version of the library and link that to the tests only. Or the tests could dynamically load the library like rviz does, but that seems harder than it needs to be. It's a bit of shame to do have the visibility macros just for testing. |
|
I agree with the visibility. I'll see how this can be changed. |
|
With 30eebad and ros/resource_retriever@4b62426 the warnings from dependencies should not be an issue anymore. In my nicer VM software (parallels) on my mac rviz will now run, but it has the issues loading the plugins that you described already. I'll work on those tomorrow, but if we clear the remaining warnings we can merge this pr in the meantime. |
|
Another CI:
|
|
The last commits should improve the picture even more:
Category C4251 is tougher. If I understand correctly, the problem arises when exporting classes which use template types. At that point, the compiler wants you to export the instantiated template class type as well. However, I believe that will almost always result in double symbol linking errors.
|
|
Another CI to check the current state:
|
|
With ros/pluginlib#97 I can now load plugins and stuff with rviz on Windows, but we can merge that separately from this one. I have an idea on how to handle those warnings, by properly exporting our specific template specializations of the STL, following this guide: Also related: |
|
So that worked, except for a few cases with For which I cannot find a solution. The other solution, which is what we do elsewhere in ROS 2, is to just dllexport small sets of things (i.e. not the whole class), which is what I do in b6dbfc2. I also disable the warning when it's within Ogre and we cannot fix it. It's a pretty low threat warning, since it only causes a problem (maybe) if you're using a different compiler and/or std library from the binaries of our stuff. This is especially unlikely given rviz being used as an application most of the time. |
|
I should say I only fixed |
|
Ok, I think I fixed all the warnings:
|
|
That's much easier than what we were trying to do. We'll fix the display tests in a followup pull request, since they don't run, yet, anyway. |
|
Yup, I'm running CI on all platforms:
|
|
I'm going to fix the issues popping up in Linux and run another CI. |
|
I had to revert a change that broke master (outside of rviz), here's new CI:
The linux uneven visibility will still be an issue if you could look at that @Martin-Idel-SI. |
|
New CI:
|
* add dependency between rviz2 and rviz_default_plugins (#149) * Move pointcloud displays to rviz_default_plugins (#153) * Move pointcloud displays to rviz_default_plugins * Move pointcloud tests to rviz_default_plugins * Adjust namespaces for pointcloud display files * Fix namespaces and import for pointcloud tests * Rename transformers folder * Fix linting errors * Move pointcloud files between CMakeLists, remove display from display_factory * Add pointcloud displays to plugins_description.xml * fix includes * add missing test dependencies * uncrustify * Revert "uncrustify" This reverts commit 03ed305. * disable uncrustify on long include statements * Fixed missing break in switch-case (#158) * Fall-through is not intended: listAppendNew sets the type to List while the call to setValue after the fall-through resets the type to Value. * Remove now obsolete function (#163) - was made obsolete by pr #136 which removed the memcopy * Reenable and fix config loading (#167) * Switch to assimp version 4.1.0 (#169) This should fix the dynimcally linked assimp build on Windows. * Feature/grid display refactoring (#165) * Move billboard_line to rviz2 for use in grid * Restore billboard functionality in Grid * Uncrustify + cpplint * Add tests for billboard_line - need to expose getChains for testing - one test documents faulty behaviour of Ogre in test * Refactoring of billboard_line - extract functions - fix functionality for huge numbers of elements per chain - add TODO comment to use internal Ogre functionality when fixed * Add tests for grid - need to expose functions for testing * Refactoring of grid * Propose additional newLine() possible in BillboardLine for ease of use - this gets rid of awkward "if(first element) don't add a new line" - simplifies usage - allows further refactoring of grid (for instance) * Minor refactoring of GridDisplay * Disable tests for now * Add functional header and std:: for "bind" * Introduce ROS interface abstraction to improve testability (#156) * Introduce interface for ros abstraction - move all public free function of ros_integration namespace to interface - add test for correct use of ros shutdown in VisualizerApp * Document recommendation for testable code * Move free functions from rclcpp_node_storage into RosAbstraction class * Rename RosAbstraction to RosClientAbstraction * Introduce RosNodeAbstraction for the node-centric part of the ros interface - also replaces the free function in get_topic_names_and_types * Cleanup: remove default arguments for virtual function * Refactor towards proper usage of RosNodeAbstraction * nitpick: alphabetical ordering * Comment only non interface classes (avoid duplicate documentation) * Cleanup: use consistent variable naming * Extract storage functionality from RosNodeAbstraction * Refactor RosNodeAbstraction towards a replacement of rclcpp::Node - a RosNodeAbstraction shall encapsulate a rclcpp::Node in the future and thus keeps track of its name (this simplifies a few functions) - for testing the rest of rviz the public api from the ros_integration namespace should be mocked - the optional storage parameter of RosNodeAbstraction is intended for unit testing of RosNodeAbstraction only and should not be needed otherwise * Fix gmock dependency * Migrate ImageDisplay (#164) * Reintroduce RosTopicProperty to MessageFilterDisplays - we can't correctly filter for message types. Added ToDo - Allow empty names in RosTopicProperty for now Refactor MessageFilterDisplay to correctly handle exceptions * Move ros_image_texture to default_plugins - Does mostly Ogre things, so definitely not rviz_common - Fix linters in RosImageTexture * Show image display in GUI and render default image Fix linters in rviz_rendering Improve naming of RenderWindow::setupSceneAfterInit() Move RenderPanel header to include folder * Let ImageDisplay extend MessageFilterDisplay to receive ROS image messages * Fix uncrustify and cpplint for ImageDisplay * Rename MessageFilterDisplay to RosTopicDisplay Remove the message filter parts currently commented out. Add comments regarding the missing message filter display * Add reliability profile to RosTopicDisplay * Introduce QueueSizeProperty * Put node from Display into _RosTopicDisplay * Initial move of ImageDisplay * Make ImageDisplay compile (except for missing image_display_base) * Reintroduce QueueSizeProperty to displays - Both ImageDisplay and PointCloud had the property previously * Offer generic way to change QoS objects * Expose ogre testing enviroment header in rviz_rendering * Add tests for ROSImageTexture * Use override instead of virtual where applicable * Extract scene setup lambda into separate method * Fix typo * Adapt interface of normalize and add tests * Interface abstraction for ROSImageTexture, replace raw pointers * Add onInitialize(string topic_name) to RosTopicDisplay This onInitialize automatically sets the default topic name to listen to * Move Image display to rviz_default_plugins * Add first tests for image display * Refactor ros_image_texture * Remove misleading onInitialize method from RosTopicDisplay * Disable tests for now * Implement review comments * Remove unnecessary check * fix memory leak (remake) (#173) * fix memory leak Signed-off-by: Chris Ye <chris.ye@intel.com> * remove vestigial example scene * use unique_ptr instead of shared_ptr and add comment * cpplint * Fix Windows build (#175) * wip * Fix visibility in rviz_common - needs to export some more symbols from rviz_common - small uncrustify changes * Fix stl_loader * Use dynamically linked OgrePlugins Don't link against Plugins anymore (loaded dynamically) Set ogre_plugin_dir to load dynamically Release and Debug need different behaviour * Fix yaml-cpp dynamic linking see jbeder/yaml-cpp#332 * Install rviz_rendering dlls * Reenable Windows tests * Move test folder to correct place - this is now consistent with rviz_common and rviz_default_plugins - fixup location of ogre_testing_environment - ogre_testing_environment needs to be exported * Fix linking against Ogre * Add zlib to be build on Windows * Change targets to unknown instead of shared * Fix billboard_line tests for Windows - need to expose symbols of billboard line for Windows - copy constructor of billboard line is not generated on Windows * Use rviz_common::UniformStringStream instead of std::stringstream * Fix linking of tests on Windows - just like rviz2.exe, tests need dll directories in %PATH% - add local install to environment variables for testing - in isolated builds, ogre resources cannot be found otherwise - enable tests in rviz_default_plugins on Windows but disable display * Fix grid_test * Linking to exported template function is problematic on Windows - this does not work with RVIZ_DEFAULT_PLUGINS_PUBLIC - delete tests for templated function - hide templated function again * Fix image_display tests * Introduce visibility control in rviz_default_plugins * run windeployqt on rviz2 binary to make it runnable after installation * extend PATH on Windows with location of libraries for vendor packages * Add missing visibility_control.hpp and fix license * log build output of dependencies to file * Fix Windows warnings * delete visibility from rviz_default_plugins * Disable deprecation warnings in Ogre * Fix dllexport warning in ros abstraction * fix linking warnings in rviz_rendering * fix linking errors * Fix warning in rviz_rendering * Reintroduce destructor to fix test * TF Display migration (#182) * Move tf_display * Make tf_display compile * Dissect tf_display into several classes * C++14 style adjustments of rviz_default_plugin tf classes - frame_selection_handler - frame_info - tf_display * Fix infinite loop when adding a display by topic * Rename tf_display folder to just tf * Expose functions for testability - BoundingRadius because it might need refactoring - getRenderOperation to allow updating Add tests to movable_text - Some tests already show bugs * Refactor movable_text - delete duplicate code in _setupGeometry() Further refactoring * Fix a couple of bugs - Line spacing could be set but did not have an effect - Using HorizontalAlignment::H_CENTER and text with spaces did not work correctly - Using VerticalAlignment::V_ABOVE did not work correctly * Extract more functions and refactoring * Set reasonable default value for spacing from the start * Refactoring of file style - Remove unnecessary variables - Delete non-virtual override that is protected - Remove duplicate method to getBoundingBox - Remove using namespace Ogre * Refactoring of tests - Expose update functionality instead of getRenderOp for testing - Change tests to approximate compare * Split methods in tf_display * Move some methods from tf_display to frame_info * Add tf_display to plugins_description * Change inheritance of MovableText to avoid visibility problems * Update Ogre to 1.10.11 (#181) * Use Ogre 1.10.11 * Fix deprecation warnings * Fix billboard_line - Ogre::BillboardLine is fixed, so we can remove manual counting - newLine() really finishes the line, thus rename to finishLine() * Fix cmake on Linux not appending _d any more * Migrate camera display (#183) * Initial move * Move display_group_visibility_properties * Make camera display compile * Add camera display to default plugins * Quickfix Segfault * First try * First working camera display * Shows the main scene from a different perspective * Misses visibility bits * Ignores caminfo status * Adds queue size property, visibility masks, listener removal on destruct Also includes a dummy caminfo for testing * Does not crash on startup, without visibility property * Reenable visibility properties * Add subscriber for CameraInfo messages * Fix linter issues * Reenable exact time mode * Implementing review comments * Extract smaller methods from large methods, cleanup Cleanup such as * correct float conversion * use lambda instead of bind * remove old comments * More method extraction and cleanup * Clean up and refactor display visibility properties * Split onInitialize method, remove obsolete comments * Fix visibility of BitAllocator * Fix Windows visibility * Default initialize forced_hidden (#187) * point to the source and bugtracker used in ros2 rather than the upstream one (#190) * Fix pointcloud2 display not accepting valid points (#189) * Fix rviz crash when removing a display (#191) The crash occurred when adding a camera display and then deleting any display that was created before adding the camera display. * print message to stdout instead of stderr (#193) * fix possible null pointer is dereferenced (#178) Signed-off-by: Chris Ye <chris.ye@intel.com> * Migrate polygon display (#194) * Initial move * Add to plugins_description.xml * Make display compile * Uncrustify * Change from uint32_t to size_t to silence warning on Windows. * Minor refactoring. * Enable alpha blending for the Polygon Display. * Let Ogre delete its render windows (#195) * Fix warnings from pluginlib (#196) * Update rviz2 installation to run it using `ros2 run` (#198) * disable RobotModel until it can be refactored to not use hard coded paths * Disable anti aliasing on Windows (#199) This fixes rendering issues on Windows when opening two or more render windows.
Solves #144