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

Fix GCC 8 compiler error. #136

Merged
merged 5 commits into from
Dec 8, 2017
Merged

Fix GCC 8 compiler error. #136

merged 5 commits into from
Dec 8, 2017

Conversation

allenh1
Copy link
Contributor

@allenh1 allenh1 commented Dec 6, 2017

GCC 8 has some rather strong feelings about the memcpy call seen here. This changes it to a for loop so that it uses the = operator to copy data.

@allenh1 allenh1 added the enhancement New feature or request label Dec 6, 2017
@allenh1 allenh1 requested a review from wjwwood December 6, 2017 21:22
@allenh1 allenh1 added the in progress Actively being worked on (Kanban column) label Dec 6, 2017
@wjwwood
Copy link
Member

wjwwood commented Dec 6, 2017

Can you provide the actual error from the compiler too?

I'll have to check this one closely, as the operator= is not necessarily equivalent to memcpy, but it might be.

@allenh1
Copy link
Contributor Author

allenh1 commented Dec 6, 2017

@wjwwood Here you go.

Process package 'rviz_rendering' with context:
--------------------------------------------------------------------------------
 source_space => /home/allenh1/ros2_ws/src/ros2/rviz/rviz_rendering
  build_space => /home/allenh1/ros2_ws/build/rviz_rendering
install_space => /home/allenh1/ros2_ws/install
   make_flags => -j8, -l8
  build_tests => False
--------------------------------------------------------------------------------
+++ Building 'rviz_rendering'
Running cmake because arguments have changed.
==> '. /home/allenh1/ros2_ws/build/rviz_rendering/cmake__build.sh && /usr/bin/cmake /home/allenh1/ros2_ws/src/ros2/rviz/rviz_rendering -DBUILD_TESTING=0 -DAMENT_CMAKE_SYMLINK_INSTALL=1 -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCMAKE_INSTALL_PREFIX=/home/allenh1/ros2_ws/install' in '/home/allenh1/ros2_ws/build/rviz_rendering'
-- The C compiler identification is GNU 8.0.0
-- The CXX compiler identification is GNU 8.0.0
-- Check for working C compiler: /home/allenh1/bin/gcc
-- Check for working C compiler: /home/allenh1/bin/gcc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /home/allenh1/bin/g++
-- Check for working CXX compiler: /home/allenh1/bin/g++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found ament_cmake: 0.0.3 (/home/allenh1/ros2_ws/install/share/ament_cmake/cmake)
-- Found PythonInterp: /usr/bin/python3 (found suitable version "3.5.2", minimum required is "3") 
-- Using PYTHON_EXECUTABLE: /usr/bin/python3
-- Override CMake install command with custom implementation using symlinks instead of copying resources
-- Found ament_cmake_gtest: 0.0.3 (/home/allenh1/ros2_ws/install/share/ament_cmake_gtest/cmake)
-- Found ament_index_cpp: 0.0.3 (/home/allenh1/ros2_ws/install/share/ament_index_cpp/cmake)
-- Found rviz_ogre_vendor: 1.0.0 (/home/allenh1/ros2_ws/install/share/rviz_ogre_vendor/cmake)
-- Setting OGRE_DIR to: '/home/allenh1/ros2_ws/install/share/rviz_ogre_vendor/cmake/../../../opt/rviz_ogre_vendor/lib/OGRE/cmake'
-- Found OGRE
--   static     : ON
--   components : Bites;HLMS;MeshLodGenerator;Overlay;Paging;Property;RTShaderSystem;Terrain;Volume
--   plugins    : Plugin_BSPSceneManager;Plugin_OctreeSceneManager;Plugin_PCZSceneManager;Plugin_ParticleFX;RenderSystem_GL;RenderSystem_GL3Plus
--   media      : /home/allenh1/ros2_ws/install/opt/rviz_ogre_vendor/share/OGRE/Media
-- Found Freetype: /usr/lib/x86_64-linux-gnu/libfreetype.so  
-- Found ZLIB: /usr/lib/x86_64-linux-gnu/libz.so (found version "1.2.8") 
-- Found OpenGL: /usr/lib/x86_64-linux-gnu/libGL.so  
-- Looking for XOpenDisplay in /usr/lib/x86_64-linux-gnu/libX11.so;/usr/lib/x86_64-linux-gnu/libXext.so
-- Looking for XOpenDisplay in /usr/lib/x86_64-linux-gnu/libX11.so;/usr/lib/x86_64-linux-gnu/libXext.so - found
-- Looking for gethostbyname
-- Looking for gethostbyname - found
-- Looking for connect
-- Looking for connect - found
-- Looking for remove
-- Looking for remove - found
-- Looking for shmat
-- Looking for shmat - found
-- Looking for IceConnectionNumber in ICE
-- Looking for IceConnectionNumber in ICE - found
-- Found X11: /usr/lib/x86_64-linux-gnu/libX11.so
-- Configuring done
-- Generating done
-- Build files have been written to: /home/allenh1/ros2_ws/build/rviz_rendering
==> '. /home/allenh1/ros2_ws/build/rviz_rendering/cmake__build.sh && /usr/bin/make -j8 -l8' in '/home/allenh1/ros2_ws/build/rviz_rendering'
Scanning dependencies of target rviz_rendering_automoc
[  4%] Automatic moc for target rviz_rendering
Generating moc_render_window.cpp
[  4%] Built target rviz_rendering_automoc
Scanning dependencies of target rviz_rendering
[  9%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/geometry.cpp.o
[ 14%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/apply_visibility_bits.cpp.o
[ 19%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/axes.cpp.o
[ 23%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/arrow.cpp.o
[ 28%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/logging.cpp.o
[ 33%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/movable_text.cpp.o
[ 38%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/object.cpp.o
[ 42%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/ogre_logging.cpp.o
[ 47%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/orthographic.cpp.o
[ 52%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/render_system.cpp.o
[ 57%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/render_window.cpp.o
[ 61%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/ogre_render_window_impl.cpp.o
[ 66%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/resource_config.cpp.o
[ 71%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/shape.cpp.o
[ 76%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/grid.cpp.o
[ 80%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/string_helper.cpp.o
[ 85%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/point_cloud.cpp.o
[ 90%] Building CXX object CMakeFiles/rviz_rendering.dir/src/rviz_rendering/point_cloud_renderable.cpp.o
[ 95%] Building CXX object CMakeFiles/rviz_rendering.dir/rviz_rendering_automoc.cpp.o
/home/allenh1/ros2_ws/src/ros2/rviz/rviz_rendering/src/rviz_rendering/point_cloud.cpp: In member function ‘void rviz_rendering::PointCloud::insertPointsToPointList(std::vector<rviz_rendering::PointCloud::Point>::iterator, std::vector<rviz_rendering::PointCloud::Point>::iterator)’:
/home/allenh1/ros2_ws/src/ros2/rviz/rviz_rendering/src/rviz_rendering/point_cloud.cpp:497:61: error: ‘void* memcpy(void*, const void*, size_t)’ writing to an object of type ‘struct rviz_rendering::PointCloud::Point’ with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Werror=class-memaccess]
   memcpy(begin, &*start_iterator, sizeof(Point) * num_points);
                                                             ^
In file included from /home/allenh1/ros2_ws/src/ros2/rviz/rviz_rendering/src/rviz_rendering/point_cloud.cpp:31:
/home/allenh1/ros2_ws/src/ros2/rviz/rviz_rendering/include/rviz_rendering/point_cloud.hpp:125:10: note: ‘struct rviz_rendering::PointCloud::Point’ declared here
   struct Point
          ^~~~~
cc1plus: all warnings being treated as errors
CMakeFiles/rviz_rendering.dir/build.make:446: recipe for target 'CMakeFiles/rviz_rendering.dir/src/rviz_rendering/point_cloud.cpp.o' failed
make[2]: *** [CMakeFiles/rviz_rendering.dir/src/rviz_rendering/point_cloud.cpp.o] Error 1
CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/rviz_rendering.dir/all' failed
make[1]: *** [CMakeFiles/rviz_rendering.dir/all] Error 2
Makefile:127: recipe for target 'all' failed
make: *** [all] Error 2

<== Command '. /home/allenh1/ros2_ws/build/rviz_rendering/cmake__build.sh && /usr/bin/make -j8 -l8' failed in '/home/allenh1/ros2_ws/build/rviz_rendering' with exit code '2'
<== Command '. /home/allenh1/ros2_ws/build/rviz_rendering/cmake__build.sh && /usr/bin/make -j8 -l8' failed in '/home/allenh1/ros2_ws/build/rviz_rendering' with exit code '2'

I'll have to check this one closely, as the operator= is not necessarily equivalent to memcpy, but it might be.

Absolutely -- I'm definitely not sure these operations are equivalent.

@anhosi
Copy link
Contributor

anhosi commented Dec 7, 2017

I believe the main point of the funciton insertPointsToPointList is the single memcpy instead of potentially many copies of single points. May be @wjwwood has more information on that.

If we replace this I would suggest to replace the whole function with

points_.insert(points_.cend(), start_iterator, stop_iterator);

@wjwwood
Copy link
Member

wjwwood commented Dec 7, 2017

@anhosi I was leaning to something similar, though I don't know how the performance will be impacted.

I'm going to test and, if working, take this pr and then make a follow up issue to investigate cleaning this particular segment of code up.

@wjwwood
Copy link
Member

wjwwood commented Dec 7, 2017

Ok, I tested this locally, and it does seem to work. To test it I ported this send_lots_of_points_node from the old rviz package and put it in the rviz2 package for now. It doesn't install, but can be run from the build folder.

To test this I opened a few shells with the ros2 workspace sourced and then I:

  • ran launch ros2_ws/install/share/dummy_robot_bringup/launch/dummy_robot_bringup.py
  • ran ./build/rviz2/test/tools/send_lots_of_points_node
  • ran rviz2_app
    • then added the PointCloud display and a Tf display (for fun) and then changed the fixed frame to world

CI:

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

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Thanks for looking into @allenh1.

@wjwwood wjwwood merged commit de76abd into ros2 Dec 8, 2017
@wjwwood wjwwood deleted the fix-gcc-8-compile branch December 8, 2017 01:56
@wjwwood wjwwood removed the in progress Actively being worked on (Kanban column) label Dec 8, 2017
anhosi added a commit to bosch-io/rviz that referenced this pull request Dec 19, 2017
- was made obsolete by pr ros2#136 which removed the memcopy
wjwwood pushed a commit that referenced this pull request Jan 3, 2018
- was made obsolete by pr #136 which removed the memcopy
wjwwood added a commit that referenced this pull request Feb 8, 2018
* 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 <[email protected]>

* 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 <[email protected]>

* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants