Initial import of bluerov2_description and bluerov2_gazebo - #1
Conversation
Assisted-by: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
|
|
||
| ```bash | ||
| cd ~/ws/src && git clone https://github.com/HonuRobotics/bluerov2.git | ||
| cd ~/ws && rosdep install --from-paths src -yi && colcon build |
There was a problem hiding this comment.
I'm naively going through the readme. I'm running in docker. Also, for context, I have this repo in my vrx src directory.
rosdep: I get the following errors:
ubuntu@hulihuli:~/vrx_ws$ rosdep install --from-paths src -yi
/usr/bin/rosdep:6: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
from pkg_resources import load_entry_point
ERROR: the following packages/stacks could not have their rosdep keys resolved
to system dependencies:
gz_waves_provider_fft: Cannot locate rosdep definition for [libimath-dev]
bluerov2_description: Cannot locate rosdep definition for [joint_state_publisher_gui]
There was a problem hiding this comment.
I think this may be on my side as I get an error when I try to re-init rosdep:
ubuntu@hulihuli:~/vrx_ws$ rosdep init
/usr/bin/rosdep:6: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
from pkg_resources import load_entry_point
ERROR: default sources list file already exists:
/etc/ros/rosdep/sources.list.d/20-default.list
Please delete if you wish to re-initialize
There was a problem hiding this comment.
I suspect you need to run rosdep update.
|
|
||
| ```bash | ||
| cd ~/ws/src && git clone https://github.com/HonuRobotics/bluerov2.git | ||
| cd ~/ws && rosdep install --from-paths src -yi && colcon build |
There was a problem hiding this comment.
nit with rosdep - I find combining the short and long form of cli options a bit confusing as I tend to assign some semantics to why which is used. For new folks, I find the long form a bit more clear- and there isn't a short form for --from-paths. And I think src is positional and having it buried in the options is also not clear (I know the tutorials do this)
Perhaps
rosdep install --from-paths --default-yes --ignore-packages-from-source src
|
|
||
| ```bash | ||
| cd ~/ws/src && git clone https://github.com/HonuRobotics/bluerov2.git | ||
| cd ~/ws && rosdep install --from-paths src -yi && colcon build |
There was a problem hiding this comment.
Is there a reason you want to use isolated install layout (colcon build) vs the merged install (colcon build --merge install)?
There was a problem hiding this comment.
Both layouts work and the tests pass with--merge-install too. I think that plain colcon build follows the ROS convention (ROS 2 tutorials, TurtleBot4, etc). Isolated is also the stricter check: merged prefixes can mask missing env hooks. As a matter of fact, our gz_waves_provider_* packages currently lack a GZ_SIM_SYSTEM_PLUGIN_PATH hook and only an isolated workspace catched it. Something I need to fix :)
| cd ~/ws/src && git clone https://github.com/HonuRobotics/bluerov2.git | ||
| cd ~/ws && rosdep install --from-paths src -yi && colcon build | ||
| source install/setup.bash | ||
| ros2 launch bluerov2_gazebo sim.launch.xml # Gazebo |
Assisted-by: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
Assisted-by: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
| if shutil.which('gz') is None: | ||
| pytest.skip('gz CLI not available') | ||
| env = dict(os.environ, GZ_PARTITION=f'test_{uuid.uuid4().hex[:8]}') | ||
| proc = subprocess.Popen(['gz', 'sim', '-s', '-r', str(WORLD)], env=env, |
There was a problem hiding this comment.
The test fixture starts gz sim -s -r directly via subprocess, completely bypassing bluerov2_gazebo/launch/sim.launch.xml. It is more an smoke test that an end-to-end test that checks the composable-node container, ros_gz_bridge, and robot_state_publisher. Not sure how that is going to run on headless standard github runners but worth having the whole machinery tested.
There was a problem hiding this comment.
Added the test to launch the container with ROS nodes: f2715b7
Assisted-by: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
j-rivero
left a comment
There was a problem hiding this comment.
First pass with some relevant comments.
| </xacro:macro> | ||
|
|
||
| <!-- Water Linked DVL-A50: native DopplerVelocityLog custom sensor, 4-beam Janus | ||
| (tilt 22.5 deg). Publishes gz.msgs.DVLVelocityTracking (no stock ros_gz |
There was a problem hiding this comment.
Commet seems outdated or wrong about the no stock ros_gz for DVL:
"""
(tilt 22.5 deg). Publishes gz.msgs.DVLVelocityTracking, bridged to
marine_acoustic_msgs/msg/Dvl by ros_gz_bridge. Requires gz-sim-dvl-system.
"""
There was a problem hiding this comment.
Fixed in af8d767. Also verified the support is released: ros_gz 3.0.9 in Lyrical carries the Dvl mapping and marine_acoustic_msgs is released there as 2.1.0-3.
| def gz(env, *args, timeout=10): | ||
| """Run a gz CLI command; return (returncode, stdout).""" | ||
| try: | ||
| out = subprocess.run(['gz', *args], env=env, capture_output=True, |
There was a problem hiding this comment.
For the gz test and the ROS test I see nothing in the output from Gazebo or ROS to be sure that there is no issues happening. It would be useful to have the output, either after the test run or live in the log.
There was a problem hiding this comment.
Added in 2cc7a12: both fixtures log the gz / ros2 launch output to a file and print the tail on failure.
|
|
||
|
|
||
| def buoyancy_ratio(root): | ||
| """Return water_density * base_link collision volume / total mass.""" |
There was a problem hiding this comment.
Here my review agent is complaining about the computation of bouyancy:
gz-sim-buoyancy-system in uniform-density mode sums every collision in the model. The test sums only base_link's box
I copy here the suggestion to get the computations correctly:
Two accessory types add collisions that the analytic sizing at bluerov2.urdf.xacro:41-48 never accounts for — it sums masses only. _claw_body (accessories.xacro:144) is attached by a fixed joint, so sdformat lumps it into base_link and its volume adds to the analytically-sized box.
| Loadout | Ratio asserted | Ratio Gazebo computes | Miss |
|---|---|---|---|
standard + camera + newton_gripper |
1.0002 | ≈1.022 | ≈110× the intended margin |
| standard + full catalog (what the test runs) | 1.0002 ± 1e-4 | 1.0308 | ≈150× |
The vehicle is briskly positive, not near-neutral — contradicting bluerov2_description/README.md:14-16 and the world comment at worlds/bluerov2_playground.sdf:35-39. The centre of buoyancy also shifts ~9.6 mm forward and 2.4 mm down, adding a pitch bias. test_buoyancy_invariant_across_loadouts (:120-131) runs the full catalog, which includes both claws — so it faithfully reproduces the bug instead of catching it.
Suggested Fix
Measure what Gazebo measures:
def collision_volume(root):
"""Sum every collision volume in the URDF, as gz-sim Buoyancy does."""
total = 0.0
for col in root.findall('.//collision/geometry'):
box, cyl, sph = col.find('box'), col.find('cylinder'), col.find('sphere')
if box is not None:
x, y, z = (float(v) for v in box.get('size').split())
total += x * y * z
elif cyl is not None:
total += math.pi * float(cyl.get('radius'))**2 * float(cyl.get('length'))
elif sph is not None:
total += 4/3 * math.pi * float(sph.get('radius'))**3
else:
raise AssertionError(f'unhandled collision geometry: {[c.tag for c in col]}')
return totalThe raise on unhandled geometry is deliberate — it makes a future mesh-collision accessory fail loudly rather than drop silently out of the sum. This test will fail today; that is the point. Then pick one: drop the <collision> from the claw macros (cost: no claw contact physics), or subtract the known claw volumes in buoyancy_volume.
There was a problem hiding this comment.
Fixed in aa7658b: the sizing now subtracts accessory collision volumes, and the test sums every collision exactly as the Buoyancy system does (your 1.0308 reproduced as 1.0309 before the fix).
| assert joint_refs <= urdf_joints | ||
| assert link_refs <= urdf_links | ||
| if shutil.which('gz') is None: | ||
| return # raw-URDF check only where the gz CLI is unavailable |
There was a problem hiding this comment.
| return # raw-URDF check only where the gz CLI is unavailable | |
| pytest.skip('gz CLI unavailable: post-lumping check cannot run') |
| <test_depend>ament_lint_common</test_depend> | ||
| <test_depend>ament_cmake_pytest</test_depend> | ||
| <test_depend>ament_index_python</test_depend> | ||
| <test_depend>python3-pytest</test_depend> |
There was a problem hiding this comment.
| <test_depend>python3-pytest</test_depend> | |
| <test_depend>python3-pytest</test_depend> | |
| <test_depend>liburdfdom-tools</test_depend> |
Nothing in CI is probably validating URDF, let's declare the test dependency
There was a problem hiding this comment.
We're checking URDF and SDF in CI.
| <inertial> | ||
| <origin xyz="0 0 0.011" rpy="0 0 0"/> | ||
| <mass value="${base_mass}"/> | ||
| <inertia ixx="0.099" ixy="0.0" ixz="0.0" |
There was a problem hiding this comment.
Seems like the footprint_y and base_mass can have a dynamic mass while the inertia here seems to be the same for different cases, is this correct?
There was a problem hiding this comment.
Fixed in 39fcd96: the hull inertia is now computed from the variant footprint (box-slab formula), so heavy gets its own tensor instead of inheriting the standard one.
| params="number xyz rpy spin visual_rpy parent:=base_link"> | ||
| <link name="thruster${number}"> | ||
| <inertial> | ||
| <mass value="0.002"/> |
There was a problem hiding this comment.
Far from my physics knowledge but agent complains worth a double check:
"""
Thruster links: 2 g mass paired with 0.001 kg·m² inertia.
These are mutually impossible: I = m·r² implies a radius of gyration of 0.707 m for a 76 mm propeller. Separately, a real T200 is ~0.344 kg in air, so six thrusters contribute 12 g instead of ~2 kg, and the whole vehicle comes out at 10.09 kg where a real standard BlueROV2 is ~11.5 kg. velocity_control masks the rotational error (applied thrust stays exact), but total mass and the buoyancy box height are both derived from these numbers.
For a 76 mm, ~0.34 kg propeller assembly: izz ≈ 0.34·0.038²/2 ≈ 2.5e-4, ixx = iyy ≈ 1.2e-4.
"""
| </inertial> | ||
| <visual> | ||
| <geometry> | ||
| <!-- Mesh is ~40x oversized as authored; 0.01 matches CentraleNantes usage. --> |
There was a problem hiding this comment.
Agent is doing some maths that are not mathching with the comment:
The comment claims the ping360 mesh is “~40x oversized as authored; 0.01 matches CentraleNantes usage”. Measuring the COLLADA vertex bbox gives 7.70 × 7.70 × 8.30 units (no element, identity node transform), so at scale="0.01" the result is 0.077 × 0.077 × 0.083 m — the mesh is authored ~100× oversized. If 40× were right the scale would have to be 0.025.
The “40×” almost certainly leaked from the hull meshes, where it is exactly right: bluerov2.dae measures 9.99 × 13.54 × 18.00 raw, and scale="0.025" yields 0.250 × 0.339 × 0.450 m against the real 0.254 × 0.338 × 0.457 m vehicle — precisely 40×. The scale values themselves are correct in both cases; only the explanation is wrong.
| files: \.(xml|xacro|sdf)$|model\.config$ | ||
| # Existing placeholder meshes reach 24 MB; tighten once the artist | ||
| # assets (with a size budget) replace them. | ||
| - id: check-added-large-files |
There was a problem hiding this comment.
Top level is excluding ^bluerov2_description/(meshes|rviz)/ are we sure that this one if checking the expected meshes files?
There was a problem hiding this comment.
Good catch, fixed in 2cc7a12: the excludes moved per-hook, so check-added-large-files now sees the meshes it exists to guard.
|
The inertia view in Gazebo is kind of weird and I can not see anything different in the rendering when I enable the collision view. Is this expected? |
Assisted-by: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
…ware Assisted-by: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
Assisted-by: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
Assisted-by: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carlos Aguero <caguero@honurobotics.com>
The collision element is only designed for buoyancy, the real collision primitives will come later from @phfatmonkey. |



BlueROV2 description and Gazebo simulation packages (ROS 2 Lyrical + Gazebo Jetty), with generated model/bridge, tests and CI.