Skip to content

Fix invisible-arrow and stale-direction bugs in ImuAccVisual#226

Merged
mintar merged 1 commit into
CCNYRoboticsLab:rollingfrom
kristkis:imu_acc_visual_zero_quat
Apr 27, 2026
Merged

Fix invisible-arrow and stale-direction bugs in ImuAccVisual#226
mintar merged 1 commit into
CCNYRoboticsLab:rollingfrom
kristkis:imu_acc_visual_zero_quat

Conversation

@kristkis
Copy link
Copy Markdown
Contributor

Summary

Fix two related bugs in ImuAccVisual that together made the acceleration arrow appear broken for raw-IMU publishers:

  1. Invisible arrow when the publisher doesn't estimate orientation. setMessage() was doing direction_ = orientation * direction_ using msg->orientation verbatim, with no check on whether the orientation is actually valid. Publishers that follow REP-145 for "orientation not estimated" either leave the quaternion at the default (0, 0, 0, 0) or set orientation_covariance[0] = -1.0. Multiplying a vector by a zero quaternion silently produces (0, 0, 0) → arrow length zero → invisible. No log, no warning.

  2. setDerotated() copy-paste bug. The setter was updating colour instead of direction (lifted from setAlpha / setColor), so toggling the "Derotate Acceleration" rviz property did nothing visible until the next message arrived.

Fix

  • Add checkQuaternionValidity() mirroring the one already used in ImuOrientationVisual (introduced in [rviz_imu_plugin] print ros_warn and give unit quaternion to ogre to … #90 on melodic but never forward-ported to the ROS 2 branches).
  • Cache the last raw accel + orientation in setMessage() so we can recompute direction_ later from the same data.
  • Factor the arrow-update logic into updateDirection() and call it from both setMessage() and setDerotated().
  • Emit a WARN on first invalid orientation + INFO on recovery, matching the orientation-visual pattern so behaviour is consistent across the two classes.

Related issues / PRs

Backport

Targeting rolling. Happy to open backport PRs for jazzy, humble, and iron if maintainers prefer, or I can update this PR to target a different branch.


Co-authored with Claude (Anthropic) during a debugging session on a separate micro-ROS project that surfaced this bug.

…ccVisual

Bugs fixed in this commit:
  * Invisible acceleration arrow for raw-IMU publishers that don't
    estimate orientation (issues CCNYRoboticsLab#86, CCNYRoboticsLab#219 likely root cause).
  * "Derotate Acceleration" checkbox didn't visibly change the arrow
    until a new message arrived.

Signed-off-by: kristkis <kisyuk.kristina@gmail.com> , Aliensense
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@mintar
Copy link
Copy Markdown
Collaborator

mintar commented Apr 27, 2026

LGTM!

One small correction of the issue description here. It says:

Add checkQuaternionValidity() mirroring the one already used in ImuOrientationVisual (introduced in #90 on melodic but never forward-ported to the ROS 2 branches).

This is not 100% correct. #90 never included imu_acc_visual.cpp, although it should have. #90 only added the fix to imu_axis_visual.cpp; this PR also applies it to imu_acc_visual.cpp. #90 was merged before the ROS2 branches were forked off, so there's nothing to forward-port.

I just had to leave this comment here because I was confused by it and it took me way too long to figure out what's going on. Maybe my future self will thank me if I ever come back to this PR.

Apart from that: Yes, targeting rolling and then back-porting is the way to go, just like you did.

A backport PR to humble/jazzy would be appreciated. No separate PRs for humble and jazzy are required; just open one PR and I'll keep the two branches in sync manually.

@mintar mintar merged commit 7e00759 into CCNYRoboticsLab:rolling Apr 27, 2026
1 check passed
@mintar
Copy link
Copy Markdown
Collaborator

mintar commented Apr 27, 2026

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants