physics classes call sensors functions

[osrf-migration]

Original report (archived issue) by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

The libgazebo_sensors library links with libgazebo_physics but not the other way around. This allows the sensor library to call physics functions. However, the physics library also calls sensor functions. Is this a circular linking problem?

$ grep -rnI sensors:: gazebo/physics/
gazebo/physics/Model.cc:676:        !sensors::SensorManager::Instance()->SensorsInitialized() &&
gazebo/physics/Link.cc:158:          sensors::create_sensor(sensorElem, this->GetWorld()->GetName(),
gazebo/physics/Link.cc:259:    sensors::remove_sensor(*iter);
gazebo/physics/Link.cc:779:    sensors::SensorPtr sensor = sensors::get_sensor(*iter);
gazebo/physics/Joint.cc:81:    sensors::remove_sensor(*iter);
gazebo/physics/Joint.cc:267:          sensors::create_sensor(sensorElem, this->GetWorld()->GetName(),
gazebo/physics/Joint.cc:340:    sensors::remove_sensor(*iter);
gazebo/physics/Joint.cc:574:    sensors::SensorPtr sensor = sensors::get_sensor(*iter);
gazebo/physics/Link.hh:377:      /// sensors::Sensor. Access to a Sensor object
gazebo/physics/Link.hh:378:      /// is accomplished through the sensors::SensorManager. This was done to
gazebo/physics/World.cc:557:      sensors::SensorManager::Instance()->SensorsInitialized())
gazebo/physics/World.cc:990:  sensors::SensorManager::Instance()->ResetLastUpdateTimes();

[osrf-migration]

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

---

Looking more into the problem, I'm starting to think that this is the root of the problems related to handsim linking in Jackie's PR and handsim issue #80. My current patch to gazebo libraries order (issue #1568) does not fix the issue.

We are not defining the proper link in cmake (phystics on sensors, which indeed,would trigger a circular dependency) so the information is not stored in the library metadata. We need a way (a design patch) to avoid this kind of trick.

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

See also the unit_test_dependencies branch where I discovered this issue.

[osrf-migration]

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

---

Yep, cmake link dependencies are transitive so we don't need to define every dependency in chain, just the direct one. I was doing the same in my testing branch (b1fb586d5a0c2cd425d54bdf4c674d0f2d85cb21).

[osrf-migration]

Original comment by John Hsu (Bitbucket: hsu, GitHub: hsu).

---

to see how we can decouple sensor dependency in physics, I am just going through listing out physics objects using sensors:

• create_sensor
• remove_sensor
• get_sensor

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

Here's a big grepped list

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

A subset of that list:

$ grep -rnI sensors:: gazebo/physics
gazebo/physics/Joint.cc:88:    sensors::remove_sensor(*iter);
gazebo/physics/Joint.cc:274:          sensors::create_sensor(sensorElem, this->GetWorld()->GetName(),
gazebo/physics/Joint.cc:347:    sensors::remove_sensor(*iter);
gazebo/physics/Joint.cc:581:    sensors::SensorPtr sensor = sensors::get_sensor(*iter);
gazebo/physics/Link.cc:169:          sensors::create_sensor(sensorElem, this->GetWorld()->GetName(),
gazebo/physics/Link.cc:270:    sensors::remove_sensor(*iter);
gazebo/physics/Link.cc:812:    sensors::SensorPtr sensor = sensors::get_sensor(*iter);
gazebo/physics/Link.hh:401:      /// sensors::Sensor. Access to a Sensor object
gazebo/physics/Link.hh:402:      /// is accomplished through the sensors::SensorManager. This was done to
gazebo/physics/Model.cc:688:        !sensors::SensorManager::Instance()->SensorsInitialized() &&
gazebo/physics/World.cc:569:      sensors::SensorManager::Instance()->SensorsInitialized())
gazebo/physics/World.cc:1008:  sensors::SensorManager::Instance()->ResetLastUpdateTimes();

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

A related question: should we make separate pkg-config and cmake config files for applications that want to link against libgazebo_client? I'm thinking of all the standalone examples, which are currently linking against libgazebo

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

That would be nice.

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

I created #1606 to track the issue of separate pkg-config and cmake config files for libgazebo_client.

[osrf-migration]

Original comment by Peter Mitrano (Bitbucket: peter_mitrano).

---

+1 I'm having a problem building gazebo on Ubuntu right now for this reason. gz fails to link, because it relies on physics and sensors, and physics contains undefined references to sensors. Is something wrong with default right now?

[osrf-migration]

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

---

AFAIK, the code is being modified to fix this circular dependency problem and all the fixes are not yet in the default branch.

Let's try some dark linker magic as a workaround:

diff -r 934cdb2c3dcfdd4814d8865fe2c9f00f4a13e580 tools/CMakeLists.txt
--- a/tools/CMakeLists.txt  Sun Aug 02 12:41:12 2015 -0700
+++ b/tools/CMakeLists.txt  Thu Oct 08 00:58:15 2015 +0200
@@ -43,6 +43,8 @@
  gazebo_msgs gazebo_common gazebo_transport gazebo_gui gazebo_physics
  gazebo_physics_ode gazebo_sensors ${QT_LIBRARIES} ${Boost_LIBRARIES})

+set_target_properties(gz PROPERTIES LINK_FLAGS "-Wl,--no-as-needed")
+
 if(HAVE_BULLET)
   target_link_libraries(gz gazebo_physics_bullet)
 endif()

My hypothesis behind this workaround: the linker flag --as--needed (enabled by default) automatically detects overlinking (using more libraries than the object code really needs) and ignore the ones not directly related to the source being linked. The gz executable is really using physics symbols (gz_log.cc file) but there is no direct reference in gz code to sensors, so the linker is ignoring sensors library thus the fail since those symbols are needed for the physics library.

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

That patch fixes the compilation for me on wily

[osrf-migration]

Original comment by Peter Mitrano (Bitbucket: peter_mitrano).

---

"dark linker magic"...my second favorite phrase. Just behind "Github or it didn't happen"

I'll try this soon, and see if it fixes it...

[osrf-migration]

Original comment by Peter Mitrano (Bitbucket: peter_mitrano).

---

This fixed it for me. Same problem with UNIT_BoxShape_TEST. Which CMake do I have to edit for that test? It's a gazebo/physics test but I can't find out where it's defined.

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

It's actually created in cmake/GazeboTestUtils.cmake, but it might be a more involved fix. I would recommend disabling test compilation for now (-DENABLE_TESTS_COMPILATION:BOOL=False).

[osrf-migration]

Original comment by Peter Mitrano (Bitbucket: peter_mitrano).

---

what about this? seems simple enough...

diff -r 24c8a7a8ba6b4d9a13dd023bacd037294fe33a96 cmake/GazeboTestUtils.cmake
--- a/cmake/GazeboTestUtils.cmake   Fri Oct 02 15:40:18 2015 -0700
+++ b/cmake/GazeboTestUtils.cmake   Thu Oct 08 18:56:31 2015 -0400
@@ -41,6 +41,8 @@
       gtest_main
       )

+    set_target_properties(${BINARY_NAME} PROPERTIES LINK_FLAGS "-Wl,--no-as-needed")
+
     add_test(${BINARY_NAME} ${CMAKE_CURRENT_BINARY_DIR}/${BINARY_NAME}
    --gtest_output=xml:${CMAKE_BINARY_DIR}/test_results/${BINARY_NAME}.xml)

[osrf-migration]

Original comment by Peter Mitrano (Bitbucket: peter_mitrano).

---

Although, on second thought, we really don't want to do this for ALL tests...

This is all just a work-around for fixing the circular dependency between sensors and physics, right? Maybe we're going to far into work-arounds.

[osrf-migration]

Original comment by Ferd Chen (Bitbucket: freason).

---

I tried to build gazebo 6.5.1 with cmake 3.3.1 but failed, it gave me the error message: [ 94%] Linking CXX executable gz ../gazebo/physics/libgazebo_physics.so.6.5.1: undefined reference to gazebo::sensors::SensorManager::~SensorManager()' ../gazebo/physics/libgazebo_physics.so.6.5.1: undefined reference togazebo::sensors::create_sensor(boost::shared_ptr, std::string const&, std::string const&, unsigned int)' ../gazebo/physics/libgazebo_physics.so.6.5.1: undefined reference to gazebo::sensors::get_sensor(std::string const&)' ../gazebo/physics/libgazebo_physics.so.6.5.1: undefined reference togazebo::sensors::Sensor::FillMsg(gazebo::msgs::Sensor&)' ../gazebo/physics/libgazebo_physics.so.6.5.1: undefined reference to gazebo::sensors::SensorManager::SensorManager()' ../gazebo/physics/libgazebo_physics.so.6.5.1: undefined reference togazebo::sensors::SensorManager::ResetLastUpdateTimes()' ../gazebo/physics/libgazebo_physics.so.6.5.1: undefined reference to gazebo::sensors::remove_sensor(std::string const&)' ../gazebo/physics/libgazebo_physics.so.6.5.1: undefined reference togazebo::sensors::SensorManager::SensorsInitialized()'

it seems that, the new cmake remove duplicate libraries automatically, and the work through patch for this bug depends on the old feature of cmake.

My suggestion is that, if Joint and Link depends on sensor, why not to move them to a new module, say physics_joint?

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

see pull request #2089, which has a fix for the default branch (gazebo7). I'm not sure if this will be fixed in gazebo6

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

• changed state from "new" to "resolved"

pull request #2089, to be included in gazebo7

[osrf-migration]

Original comment by John Hsu (Bitbucket: hsu, GitHub: hsu).

---

I propose we get this fixed for gazebo6 and for gazebo4 as those will fail if one is trying to compile them with cmake 3 on Trusty systems (as is the case when trying to get it working for another project stack).

Unless there're objections, I'll start back porting.

Thanks.

[osrf-migration]

Original comment by John Hsu (Bitbucket: hsu, GitHub: hsu).

---

reference similar failure in gazebo_4.1 branch:

Linking CXX executable UNIT_BoxShape_TEST
cd /home/hsu/projects/gazebo_4/build_release/gazebo/physics && /usr/bin/cmake -E cmake_link_script CMakeFiles/UNIT_BoxShape_TEST.dir/link.txt --verbose=1
/usr/bin/c++    -s -O3 -DNDEBUG -msse4.2 -msse4.1 -mssse3 -msse3 -mfpmath=sse -msse -msse2   -Wall -Wextra -Wno-long-long -Wno-unused-value -Wno-unused-value -Wno-unused-value -Wno-unused-value -Wfloat-equal -Wshadow -Winit-self -Wswitch-default -Wmissing-include-dirs -pedantic -fvisibility=hidden -fvisibility-inlines-hidden  -s -O3 -DNDEBUG -msse4.2 -msse4.1 -mssse3 -msse3 -mfpmath=sse -msse -msse2     CMakeFiles/UNIT_BoxShape_TEST.dir/BoxShape_TEST.cc.o  -o UNIT_BoxShape_TEST  -L/home/hsu/projects/gazebo_4/build_release/test -rdynamic -Wl,-Bstatic -lgtest -lgtest_main -Wl,-Bdynamic ../../test/libserver_fixture.a ../libgazebo.so.4.1.3 -lpthread ../sensors/libgazebo_sensors.so.4.1.3 libgazebo_physics.so.4.1.3 ode/libgazebo_physics_ode.so.4.1.3 ../../deps/opende/libgazebo_ode.so.4.1.3 ../../deps/opende/GIMPACT/libgazebo_gimpact.so.4.1.3 ../../deps/opende/ou/libgazebo_opende_ou.so.4.1.3 -lccd ../../deps/opende/OPCODE/libgazebo_opcode.so.4.1.3 simbody/libgazebo_physics_simbody.so.4.1.3 -lSimTKsimbody -lSimTKmath -lSimTKcommon -lblas -llapack -lblas -llapack -ldl -lOgreRTShaderSystem -lOgreMain -lpthread -lOgreTerrain -lOgrePaging -lOgreMain -lpthread -lOgreTerrain -lOgrePaging ../rendering/libgazebo_rendering.so.4.1.3 ../rendering/skyx/libgazebo_skyx.so.4.1.3 ../rendering/selection_buffer/libgazebo_selection_buffer.so.4.1.3 -lCEGUIBase -lCEGUIOgreRenderer -lX11 ../rendering/deferred_shading/libgazebo_rendering_deferred.so.4.1.3 -lOgreRTShaderSystem -lOgreTerrain -lOgrePaging -lOgreMain -lGLU -lGL ../util/libgazebo_util.so.4.1.3 ../transport/libgazebo_transport.so.4.1.3 ../msgs/libgazebo_msgs.so.4.1.3 ../common/libgazebo_common.so.4.1.3 -ldl -lfreeimage -ltinyxml -lavcodec -lavformat -lavutil -lcurl -lswscale -ltar /home/hsu/local_sdformat_2/lib/libsdformat.so -lpthread -lrt -lgts -lm -lgthread-2.0 -lgmodule-2.0 -lglib-2.0 -lopenal -lgdal ../math/libgazebo_math.so.4.1.3 -lprotobuf -lboost_thread -lboost_signals -lboost_system -lboost_filesystem -lboost_program_options -lboost_regex -lboost_iostreams -lboost_date_time -lpthread -ltbb -Wl,-rpath,/home/hsu/projects/gazebo_4/build_release/test:/home/hsu/projects/gazebo_4/build_release/gazebo:/home/hsu/projects/gazebo_4/build_release/gazebo/sensors:/home/hsu/projects/gazebo_4/build_release/gazebo/physics:/home/hsu/projects/gazebo_4/build_release/gazebo/physics/ode:/home/hsu/projects/gazebo_4/build_release/deps/opende:/home/hsu/projects/gazebo_4/build_release/deps/opende/GIMPACT:/home/hsu/projects/gazebo_4/build_release/deps/opende/ou:/home/hsu/projects/gazebo_4/build_release/deps/opende/OPCODE:/home/hsu/projects/gazebo_4/build_release/gazebo/physics/simbody:/home/hsu/projects/gazebo_4/build_release/gazebo/rendering:/home/hsu/projects/gazebo_4/build_release/gazebo/rendering/skyx:/home/hsu/projects/gazebo_4/build_release/gazebo/rendering/selection_buffer:/home/hsu/projects/gazebo_4/build_release/gazebo/rendering/deferred_shading:/home/hsu/projects/gazebo_4/build_release/gazebo/util:/home/hsu/projects/gazebo_4/build_release/gazebo/transport:/home/hsu/projects/gazebo_4/build_release/gazebo/msgs:/home/hsu/projects/gazebo_4/build_release/gazebo/common:/home/hsu/local_sdformat_2/lib:/home/hsu/projects/gazebo_4/build_release/gazebo/math 
libgazebo_physics.so.4.1.3: undefined reference to `gazebo::sensors::SensorManager::~SensorManager()'
libgazebo_physics.so.4.1.3: undefined reference to `gazebo::sensors::get_sensor(std::string const&)'
libgazebo_physics.so.4.1.3: undefined reference to `gazebo::sensors::Sensor::FillMsg(gazebo::msgs::Sensor&)'
libgazebo_physics.so.4.1.3: undefined reference to `gazebo::sensors::SensorManager::SensorManager()'
libgazebo_physics.so.4.1.3: undefined reference to `gazebo::sensors::create_sensor(boost::shared_ptr<sdf::Element>, std::string const&, std::string const&, unsigned int)'
libgazebo_physics.so.4.1.3: undefined reference to `gazebo::sensors::SensorManager::ResetLastUpdateTimes()'
libgazebo_physics.so.4.1.3: undefined reference to `gazebo::sensors::remove_sensor(std::string const&)'
libgazebo_physics.so.4.1.3: undefined reference to `gazebo::sensors::SensorManager::SensorsInitialized()'
collect2: error: ld returned 1 exit status
make[2]: *** [gazebo/physics/UNIT_BoxShape_TEST] Error 1
make[2]: Leaving directory `/home/hsu/projects/gazebo_4/build_release'
make[1]: *** [gazebo/physics/CMakeFiles/UNIT_BoxShape_TEST.dir/all] Error 2
make[1]: Leaving directory `/home/hsu/projects/gazebo_4/build_release'
make: *** [all] Error 2

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

I don't think it can be back-ported without breaking ABI

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

There are member variables added to SensorManager

[osrf-migration]

Original comment by John Hsu (Bitbucket: hsu, GitHub: hsu).

---

Thanks I guess the proposal is not physically feasible.

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

• set version to "all"

[osrf-migration]

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).

---

• changed state from "resolved" to "closed"