feat: per-topic message transforms; Cloudini point cloud compression - #16
Open
facontidavide wants to merge 19 commits into
Open
facontidavide wants to merge 19 commits into
facontidavide wants to merge 19 commits into
Conversation
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
std::span is needed by the message transform interface, and cloudini_lib exports cxx_std_20 publicly, so the standard must not depend on whether that optional dependency is found. Humble and Jazzy (pixi) build warning-free, 271/271 tests pass on both. FastDDS standalone not rebuilt here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
Adds the MessageTransform abstract interface and TransformFactory struct that later tasks build on: TransformSet (profile parsing and matching), the ROS2 ingest hook, and the strip/cloudini transforms. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
TransformSet parses a JSON transform profile into ordered match_type / match_topic rules bound to named factories, validating every rule at startup (unknown transform, rejected params, invalid regex, a type the transform declares it doesn't accept). bind() applies first-match-wins and caches the per-topic BoundTransform, rebinding when a topic's source type changes; BoundTransform::run() applies the transform and tracks samples/drops/bytes/latency counters for stats_summary(). Also reformats app/include/pj_bridge/message_transform.hpp per clang-format (pre-commit was not run before the previous commit). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
Adds TransformingTopicSource, a TopicSourceInterface decorator that rewrites a matched topic's advertised type/schema to the transform's output, tagging the original type on TopicInfo::source_type (new, defaulted, last member). BridgeServer emits source_type on get_topics entries when non-empty and advertises the message_transforms capability. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
Review findings on TransformSet: an exception from apply() or a factory, or a null transform, now costs a sample / leaves the topic untransformed instead of unwinding into the ingest executor. A topic-only rule whose transform does not accept the type is skipped rather than ending the search, so later rules (the strip_large_messages sugar) still apply. Adds caching, stats, validation and two-thread tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
Ros2SubscriptionManager now takes a std::shared_ptr<TransformSet> instead of a strip_large_messages bool. subscribe() looks up the topic's bound transform (created earlier via TransformSet::bind(), e.g. by TransformingTopicSource::get_topics()), subscribes with the transform's source_type instead of the advertised output type, and runs the transform on a span over the rcl buffer directly into a fresh output vector -- no copy of the input. On transform failure the sample is DROPPED instead of forwarding the original bytes, since the client was already told the output type at subscribe time. `strip` is now one MessageTransform, adapting MessageStripper. main.cpp wires it up in the next commit; for now it passes nullptr so the tree builds. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
… sugar New `transform_profile` string parameter (default "", meaning no profile). If set, main.cpp reads and parses it into a TransformSet (built-in factory: "strip"), wraps Ros2TopicSource in a TransformingTopicSource, and passes the TransformSet to Ros2SubscriptionManager. Any parse or validation failure exits 1 with a descriptive message. `strip_large_messages: true` is now sugar: after the profile's rules (if any) are loaded, one `strip` match_type rule is appended per MessageStripper::strippable_types(), so an explicit profile rule for a type always wins. Transform statistics are logged at shutdown. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
Also from review: strip rejects an empty message before touching the rcl buffer, and a factory that cannot set up a topic is logged once, not on every topic poll. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
sensor_msgs/PointCloud2 -> point_cloud_interfaces/CompressedPointCloud2 on the CDR bytes, second stage NONE (the frame is zstd-compressed already). cloudini_lib is optional: find_package first, otherwise fetched (1.3.0, pinned hash) and built as a plain static library. Its ament branch, its own zstd lookup and its install rules are kept out of this project. package.xml: no cloudini_lib dependency yet (rosdep key availability per distro not checked). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
Document the message-transform feature (transform_profile, strip and cloudini transforms, wire-protocol additions, startup/runtime error behavior) in docs/API.md, README.md, and CLAUDE.md; add a Forthcoming section to CHANGELOG.rst and an example transform_profile.example.json. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
…CMake block Final review findings: - a PointCloud2 whose width*height*point_step disagrees with its data, or with a field past point_step, is dropped instead of reaching an encoder that trusts those values (over-read / undecodable frame) - link whichever target an installed cloudini_lib exports (cloudini:: for a standalone install, cloudini_lib:: for an ament one) and require >= 1.2 - keep PCL and Cloudini's forced CMAKE_BUILD_TYPE out of this project; give data_path.hpp its own directory (Cloudini writes one to the same path) - docs: resolution applies to every FLOAT32 field, limitations, offline builds Verified: 302 tests with Cloudini, 299 with -DPJ_BRIDGE_FETCH_CLOUDINI=OFF. Not verified: the ament-installed cloudini_lib path (none available here). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
On a CI runner the main thread's 2000 iterations finished before the ingest thread had run once, so the final samples > 0 check failed. The loop now continues until the two threads have actually overlapped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
…ying
Refactors (behaviour-preserving, 302 existing tests unchanged in intent):
- output_type/output_schema move from TransformFactory to virtuals on
MessageTransform with identity defaults; the factory is accepts/check_params/
create. Removes the identity lambdas in strip and the tests, and an unused
source_type argument.
- Rule::params defaults to {}; bind() guards only what can throw.
- main.cpp: load_transforms() throws into the existing handler instead of three
copies of log/shutdown/return; append_strip_rules() replaces the loop that
main.cpp and a test both wrote.
- CMake: the Cloudini fetch is a function(), so nothing needs restoring.
- tests: shared fake_transform.hpp and one publish_until() helper.
Fix, test first (SkippedRuleWarningDoesNotHideALaterSetupError): the log-once
set was keyed by topic, so a 'does not accept' warning swallowed a later 'could
not be set up' error for the same topic. Now keyed by (topic, transform) and
cleared when a topic is rebound.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
…validation Cloudini 1.3.1 (facontidavide/cloudini#141, #142) can be embedded as-is and validates malformed clouds itself, so this removes what compensated for that: the scoped fetch function (hidden ament_cmake, disabled zstd/PCL lookups, undone CMAKE_BUILD_TYPE, FetchContent_Populate + EXCLUDE_FROM_ALL), the data_path.hpp relocation, and check_cloud() in the transform. The InconsistentCloudMetadataIsRejected test stays and now covers the upstream check. Clean rebuild against the released tarball: 303 tests, static link, no PCL, nothing of Cloudini installed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
…is always a startup error A rule matching by topic name alone could only be checked against a topic's type once that topic was discovered, so pairing e.g. cloudini with a non point cloud type surfaced as a runtime warning. Checking against the topics present at startup would depend on launch order (DDS discovery is incomplete when the bridge starts, and topics can appear later). match_type is now required and match_topic only narrows it: accepts(match_type) is verified when the profile is parsed, deterministically and independent of what is running. The runtime skip-and-warn path is removed, since the situation can no longer be expressed. Test first: All/TransformSetBadProfileTest/topic_only_rule failed before the change. The two tests describing the runtime skip are replaced by RuleNeverAppliesToATopicOfAnotherType. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
From the review of the mandatory-match_type change: - every rejected profile is now tested for its REASON, not just for being rejected (a case could otherwise pass for the wrong reason); adds the empty match_type case - 'does not accept type' shows the expected spelling (the likely mistake is the short 'sensor_msgs/PointCloud2'); an empty match_type and a wrongly typed key get accurate wording; main() names the profile file - the log-capturing test restores the default logger on every exit path (its sink pointed at a stack stream) and counts the message, not newlines - setup_failed_ replaces the (topic, transform) log-once set and its helper, over-general now that one call site remains - the implementation plan is marked historical Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Operator-configured, per-topic message transforms for the ROS2 backend, with two implementations:
cloudini—sensor_msgs/msg/PointCloud2→point_cloud_interfaces/msg/CompressedPointCloud2using Cloudini (lossy at a configurable resolution). Cloudini's own second compression stage is disabled: the bridge already zstd-compresses every frame.strip— the existingMessageStripperbehind the new interface.strip_large_messages: truekeeps working as sugar for strip rules appended after the profile's rules.Enabled with one new parameter,
transform_profile(path to a JSON file, empty = off). With no profile andstrip_large_messages: falsenothing is created and behaviour is unchanged. Design and rationale:docs/superpowers/specs/2026-09-20-message-transforms-design.md; user documentation:docs/API.md→ "Message Transforms".{"transforms": [{"match_type": "sensor_msgs/msg/PointCloud2", "transform": "cloudini", "params": {"resolution": 0.001}}]}How it works
TransformSetparses the profile (ordered rules, first match wins;match_typeis required and exact,match_topicis an optional full-match regex that narrows it) and owns one transform instance per topic plus its counters. The whole profile is validated at launch, before the WebSocket port opens: every mistake — including a transform paired with a type it cannot handle, e.g.cloudinion anImage— exits 1 with a message naming the rule.match_typeis mandatory precisely so that this check does not depend on which topics happen to be discovered at startup.TransformingTopicSourcedecorates the topic source, so a transformed topic is advertised with its output type and schema. The wire protocol is unchanged; the only additions are an optionalsource_typeonget_topicsentries and amessage_transformscapability, both ignored by the current PlotJuggler plugin (checked against its source).Ros2SubscriptionManagersubscribes with the topic's real type and runs the transform on astd::spanover the rcl buffer — the raw message is never copied — writing straight into the vectorMessageBufferkeeps. A failed transform drops the sample (counted, throttled warning); it is never forwarded raw, since the client was told the output type.Measured (pinned cores,
performancegovernor, 4 lidars × 10 Hz ≈ 52 MiB/s raw, 1 client, 3 runs each)cloudini1 mmcloudini1 mm +viz_preprocessingLossless in every run (300/300 clouds per lidar in 30 s, zero drops). Bandwidth halves at roughly equal CPU: the encode time on the ingest thread is paid back by zstd having a third of the bytes to compress. Synchronous encoding was sufficient, so no worker thread was added.
Build changes
std::span;cloudini_libalso exportscxx_std_20). First commit on the branch, so CI isolates it.find_package(cloudini_lib 1.3.1)first, otherwise fetched (1.3.1, pinned hash) with a plainFetchContent_MakeAvailableand linked statically. Embedding it cleanly and validating malformed clouds needed upstream fixes, released as Cloudini 1.3.1 (fix: reject inconsistent point clouds; make ros_message_definitions.hpp includable twice facontidavide/cloudini#141, #142); the workarounds this PR first carried for that are gone.-DPJ_BRIDGE_FETCH_CLOUDINI=OFFbuilds without thecloudinitransform (needed for builds without network access).Behaviour changes
strip: a message that fails to strip is now dropped instead of forwarded untouched (uniform transform failure policy).Testing
-DPJ_BRIDGE_FETCH_CLOUDINI=OFF, where a profile namingcloudinifails at startup with "unknown transform"; TSAN clean on the new code (includes a two-threadrun()vsbind/find/statstest).352cafb) and a final one on the mandatory-match_typechange. Bugs found along the way were reproduced by a failing test before being fixed.Not verified / known limitations
CompressedPointCloud2parser (pj-official-plugins,feat/compressed-pointcloud) — not tested here.cloudini_libpath (targetcloudini_lib::cloudini_lib) is handled but could not be exercised locally.package.xmldoes not depend oncloudini_libyet, so ROS buildfarm debs (no network) would lack the transform until that rosdep key exists; GitHub-built debs, conda packages and AppImages get it through the fetch.🤖 Generated with Claude Code
https://claude.ai/code/session_01711BZcEtp3KsoF9uZHNFME