refactor(autoware_traffic_light_visualization): separate the ROI drawing from the node

[kentdotn]

Description

Recommended: read this PR commit by commit — the table below links each one and says what it is for. It is one refactoring in five commits, each of which stands on its own and leaves the package building and every test passing. Three of them are preparation, the fourth is the move, and the fifth is a rename added after review. The combined diff is mostly relocated code, so reading it at once costs more and shows less.

Purpose

TrafficLightRoiVisualizerNode drew its debug image inside its callbacks, so nothing about the drawing could be tested without a ROS context and the characterization suite had to drive the node over real topics for every case. This PR separates the drawing from the node: TrafficLightRoiVisualizer (src/traffic_light_roi_visualizer/roi_visualizer.hpp) takes the input image and the detection messages and hands back the image to publish; the node keeps parameters, the synchronizers, the lazy subscription and the two publishers. Unit tests for the separated logic, and the reduction of the characterization suite to a node integration test, follow in the next PRs.

The split follows traffic_light_map_visualizer, the sibling node in this same package: logic beside node, helpers in the source file's anonymous namespace rather than the header, one library for both.

No behavior changes. The characterization tests added in #13392 pass at every commit, and the separation commit does not touch the test file at all.

The commits

Two kinds of commit, and they want different attention:

• mechanical — code moved or renamed, nothing new. The only question is whether anything changed on the way, which git show --stat and a skim answer.
• design — a decision was made about the boundary: what the logic is responsible for, what crosses it, what its interface looks like. These are worth arguing about.

Commits 2 and 3 are here because of what the move would otherwise inherit. Both decide something about what the separated object is allowed to know and to touch — the kind of thing that costs an interface change if it is discovered after the move rather than before it.

#	Commit	Kind	What to look for
1	rename the ROI visualizer internals to snake_case	mechanical	The package's own identifiers were camelCase against the Autoware convention. The same commit splits createRect() into draw_roi_with_id() and draw_roi_with_label(): the two overloads were told apart only by whether the third argument was a cv::Scalar or a ClassificationResult, so a call site did not say what would be drawn. They draw different things — a frame with the traffic light id written next to it, and a frame with a label box holding the shape icon and the confidence. Large, but a rename plus one split.
2	pass the shape image directory into the drawing	design	Who decides where the shape icons are. load_shape_image() held a function-local static const directory and called ament_index_cpp::get_package_share_directory() itself, so the drawing knew how the package is installed. That is the node's knowledge: it resolves the directory once in its constructor, and the three drawing functions take it as const std::string & image_dir. The drawing has to stop reaching for the package layout before it can move out of the node.
3	look up the label words without mutating the node	design	Whether the drawing has to be mutable. state2label_ is read with operator[], which inserts a default-constructed entry for any code the table does not list — the lookup writes. Harmless where it sits, but the drawing is what performs the lookup, so after the move the separated object would have to be non-const with non-const entry points, and the node would hold it as mutable state. The table becomes a free function with a function-local static const std::map and find(), which answers with the same empty string for an unlisted code without inserting.
4	separate the ROI drawing from the node	design	Where the line is drawn. Two entry points, visualize() and visualize_with_rough_rois(), both const, which commit 3 is what bought. They stay separate because they are different algorithms, and which one runs is a property of the synchronizer the node picked, not of the drawing. What was a private member of the node stays a member of the visualizer; what was static, and so carried no state, becomes a free function in the anonymous namespace rather than something the header declares. The bodies are the former callback code, unchanged — the decisions are in the interface, not in the diff of the bodies.
5	name the node file after the node	mechanical	Added after review, at @a-maumau's suggestion. Every other traffic light package pairs <logic>.cpp with <logic>_node.cpp - the classifier, the fine detector, the map based detector, the selector, the arbiter, the multi camera fusion, and traffic_light_map_visualizer in this very package. node.cpp reads fine on its own and not at all next to the others. A rename and nothing else; the component plugin name is untouched.

Two consequences of commit 4 worth naming

• The RGB8 conversion moves with the drawing, and the entry points hand back a sensor_msgs::msg::Image::SharedPtr rather than a cv_bridge pointer. The visualizer owns the image it produces instead of writing into a buffer the node half-built, and the node gets something it can publish directly.
• A latent crash goes with it. The node used to keep a cv_bridge::CvImagePtr that stays null when the conversion throws, log the failure, and then fall through to a publish that dereferences it — reproducible by feeding an image whose encoding cv_bridge cannot convert to RGB8, which logged Could not convert from '...' to 'rgb8'. and then segfaulted, taking the whole traffic_light_node_container with it. Nothing was published on that path either way, so no behavior is lost; the publish now sits inside the try, so the crash is not something to remember to avoid.

Left alone on purpose

Following the plan's rule that nothing which can wait until after the unit tests should be done here:

• The stale [[maybe_unused]] on an argument image_roi_callback() does use, use_high_accuracy_detection missing from the parameter file and the schema, and the README naming the fine detector as the source of ~/input/rois when two of the three shipped launch configurations feed it from somewhere else.
• The empty string state_to_label() answers with for a code no table entry lists, which commit 3 keeps as it is. It reaches the output: an unlisted color makes the label read -circle, and str_to_color("") then takes the same fallback an UNKNOWN color does, while an unlisted shape makes it read red- and no icon is drawn. unknown would be the better answer than an empty string, and the table already has the word - but that changes what is drawn, so it is not this PR's business.
• draw_roi_with_id() and draw_roi_with_label() return a bool that is always true and that no caller reads, and both loops copy each ROI by value.

Where the line is, checked

roi_visualizer.{hpp,cpp} include no rclcpp, image_transport, ament_index_cpp or message_filters. cv_bridge is used, but only in the source file, and its header pulls in sensor_msgs and OpenCV rather than rclcpp.

Related links

Parent Issue:

• None

Characterization tests this PR builds on: #13392

How was this PR tested?

From the CI-equivalent dev container (ghcr.io/autowarefoundation/autoware:universe-dependencies-jazzy):

colcon build --packages-select autoware_traffic_light_visualization \
  --cmake-args -DCMAKE_BUILD_TYPE=RelWithDebInfo
colcon test --packages-select autoware_traffic_light_visualization \
  --event-handlers console_direct+

• Each commit builds and passes the tests on its own: 104 tests, 0 failures, at every commit.
• clang-format, cpplint and the rest of the repository's pre-commit hooks pass.
• The crash described above was reproduced against a running node before the change and confirmed gone after it: the node now logs the conversion failure, drops that frame and draws normally again on the next convertible image.

Notes for reviewers

• Commit 4 reads as 383 insertions and 255 deletions, but roi_visualizer.cpp is the former node code relocated; --color-moved shows most of it as a move.
• The only edits to the characterization test are in commit 1: the identifier names it mentions in comments. No assertion or expected value has been changed, and commits 2 to 4 do not touch the file.

Interface changes

None. Topics, parameters and the component plugin name are unchanged.

Effects on system behavior

The node no longer crashes when cv_bridge cannot convert the incoming image to RGB8; it logs and skips that frame, as described above. Nothing else changes.

🤖 Generated with Claude Code

[github-actions]

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

• You've checked our contribution guidelines.
• Your PR follows our pull request guidelines.
• All required CI checks pass before marking the PR ready for review.

[codecov]

Codecov Report

❌ Patch coverage is 51.37615% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 20.74%. Comparing base (a6d2b0f) to head (54f0421).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/traffic_light_roi_visualizer/roi_visualizer.cpp	56.94%	1 Missing and 30 partials ⚠️
...on/src/traffic_light_roi_visualizer/shape_draw.hpp	18.18%	9 Missing ⚠️
...lization/src/traffic_light_roi_visualizer/node.cpp	53.33%	0 Missing and 7 partials ⚠️
...on/src/traffic_light_roi_visualizer/shape_draw.cpp	44.44%	0 Missing and 5 partials ⚠️
...rc/traffic_light_roi_visualizer/roi_visualizer.hpp	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13434   +/-   ##
=======================================
  Coverage   20.74%   20.74%           
=======================================
  Files        2058     2060    +2     
  Lines      142998   143007    +9     
  Branches    52628    52628           
=======================================
+ Hits        29663    29669    +6     
  Misses      89557    89557           
- Partials    23778    23781    +3     

Flag	Coverage Δ		*Carryforward flag
daily	?
full-suite	20.72% <ø> (-0.02%)	⬇️	Carriedforward from a6d2b0f

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sasakisasaki]

Reviewed. Not only the core logic separation is performed, and also the CodeScene scan result becomes much better. Nice refactoring! 👍

[a-maumau]

thank you for the refactor! LGTM too!

Just a very minor comment.
if we follow the other traffic light packages (e.g. classifier: https://github.com/autowarefoundation/autoware_universe/tree/b4b689e9a840ea6e4fb4f5ca6f74625ad7dbc580/perception/autoware_traffic_light_classifier/src ), we should change node.cpp to something like [traffic_light_]roi_visualizer_node.cpp which might not need to be in this PR though.

[kentdotn]

@a-maumau
Thank you for your review. I agreed the file name change and have pushed another commit.

I chose the file name roi_visualizer_node.cpp so that the name obeys <logic>_node.cpp rule; The sibling traffic_light_map_visualizer node is implemented with TrafficLightVisualizer and TrafficLightMapVisualizerNode classes, and the files are named traffic_light_visualizer and traffic_light_visualizer_node, not including map. So the naming based on its logic implementation would sound natural.