Conversation
…w. needs develop branch of osgviz
viz/PluginLoader.cpp
Outdated
| pluginNames->push_back("RigidBodyStateSE3Visualization"); | ||
| pluginNames->push_back("WrenchVisualization"); |
There was a problem hiding this comment.
Wrong indentation (happens elswhere in the patch).
Rock style guide is 4 space indent, no tabs. I'm guessing that you are using tabs.
viz/WrenchModel.cpp
Outdated
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Please avoid unecessary leading/trailing lines.
| // update transform | ||
| osg::Vec3d force(wrench.force.x(), wrench.force.y(), wrench.force.z()); | ||
| Q.makeRotate(osg::Vec3d(0,0,1), force); | ||
| //force_node->setScale(osg::Vec3f(resolution, resolution, resolution*force.length())); |
There was a problem hiding this comment.
Don't commit commented-out code. If it is commented out, it's not part of the implementation and just adds noise.
|
Not a requirement, but we've started to create a while ago "interactive" unit tests for visualization, that are in Would you consider creating those for these two visualizers ? It would really help. |
| DEPS base-types | ||
| LIBS ${Boost_SYSTEM_LIBRARY} | ||
| DEPS_PKGCONFIG base-logging | ||
| DEPS_PKGCONFIG base-logging PrimitivesFactory osgViz |
There was a problem hiding this comment.
I really believe we should consider other alternatives before adding osgViz as a "hard" dependency in base/types.
This is also breaking rock-core's build.
There was a problem hiding this comment.
In practice, base/types already depends on gui/vizkit3d which depends on osgViz. The (optional) dependency should definitely be explicitly added to the manifest, though.
There was a problem hiding this comment.
Yes, that will be a problem in constrained environments (i.e embedded computers running iodrivers_base based drivers). Can we please don't do that?
There was a problem hiding this comment.
The keyword was optional ;-). The dependency on osgViz that this PR adds is also optional as the whole viz/ folder is disabled by the Rock CMake macros if vizkit3d is not available or if ROCK_VIZ_ENABLED is set to OFF.
base/types already depends optionally on gui/vizkit3d. In constrained environments, you usually exclude gui/.* which removes that particular dependency
There was a problem hiding this comment.
the whole viz/ folder is disabled by the Rock CMake macros if vizkit3d is not available or if ROCK_VIZ_ENABLED is set to OFF
I missed that, sorry. I guess I was misled by the breaking build (which is probably breaking for some other reason I didn't look into). Did this pass on your CI?
There was a problem hiding this comment.
-- Checking for module 'PrimitivesFactory'
-- No package 'PrimitivesFactory' found
CMake Error at /usr/share/cmake-3.10/Modules/FindPkgConfig.cmake:415 (message):
A required package was not found
Call Stack (most recent call first):
/usr/share/cmake-3.10/Modules/FindPkgConfig.cmake:593 (_pkg_check_modules_internal)
/buildbot/squidbot-build/build/install/base/cmake/share/rock/cmake/Rock.cmake:288 (pkg_check_modules)
/buildbot/squidbot-build/build/install/base/cmake/share/rock/cmake/Rock.cmake:383 (rock_find_pkgconfig)
/buildbot/squidbot-build/build/install/base/cmake/share/rock/cmake/Rock.cmake:649 (rock_target_definition)
/buildbot/squidbot-build/build/install/base/cmake/share/rock/cmake/Rock.cmake:811 (rock_library_common)
viz/CMakeLists.txt:11 (rock_vizkit_plugin)
This is when building base/types
There was a problem hiding this comment.
I don't know what PrimitivesFactory is or where it comes from. I was looking at osgViz since it was what you mentioned in your comment.
Didn't check CI yet.
There was a problem hiding this comment.
PrimitivesFactory is a module in osgViz
3 participants
Added vizkit3d plugin for Wrenches and new RigidBodyStateSE3