Skip to content

refactor(messages): consolidate generated message utilities - #4800

Open
TomCC7 wants to merge 14 commits into
cc/feat/cdr-unit-viewersfrom
cc/refactor/cdr-sensor-value-helpers
Open

TomCC7 wants to merge 14 commits into
cc/feat/cdr-unit-viewersfrom
cc/refactor/cdr-sensor-value-helpers

Conversation

@TomCC7

@TomCC7 TomCC7 commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

Consolidate generated-message utilities in one review layer before migrating their consumers. This PR absorbs the geometry and pointcloud additions from #4801 and #4802, plus the occupancy and per-point coordinate helpers formerly introduced with mapping.

Use domain modules as namespaces: image.from_array, image.view, image.to_rgb, image.to_jpeg, image.to_compressed, camera_info.intrinsic_matrix, geometry.quaternion_from_euler, pointcloud.from_xyz, pointcloud.transform, and occupancy.from_array. Generated classes remain wire values, without wrappers, mixins or package re-exports.

Borrowed views are read-only and retain their buffers; source mutations remain visible. Array conversions return independent writable storage. JPEG bytes and CompressedImage messages have distinct APIs. Resize and downsample document when they return the original input. Pointcloud construction now always owns its float32 input, and stamp arrays are independent copies. The unused message_header helper is removed from time.py.

Validation: 225 utility tests passed; focused mypy, Ruff lint and formatting passed. No dependency installation was needed.

Base: #4733. Next: #4734 for consumer migration. #4803 is closed and its buffer behavior changes have been removed from the downstream stack. #4801 and #4802 are superseded by this PR.

The dedicated .github/workflows/cdr-native.yml is removed throughout this stack. Test sources remain; CDR integration CI will be designed separately.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go2 replay realtime (arm64)

Details
Benchmark suite Current: 6587949 Previous: 0ea05f6 Ratio
peak memory 1619.074 MB 1699.445 MB 0.95
peak threads 351 threads 357 threads 0.98
network (transport) 2724.794 MB 2725.914 MB 1.00
disk write 2.129 MB 1.469 MB 1.45
instructions 117.174 G 136.571 G 0.86

This comment was automatically generated by workflow using github-action-benchmark.

Comment thread dimos/message_utils/time.py Outdated
Comment thread dimos/message_utils/image.py Outdated
@TomCC7 TomCC7 changed the title refactor(messages): add image, calibration and time value adapters refactor(messages): consolidate generated message utilities Oct 26, 2026
@TomCC7
TomCC7 removed this pull request from stack #4804 October 26, 2026 19:10
@TomCC7
TomCC7 added this pull request to stack #4845 October 26, 2026 19:11
@TomCC7
TomCC7 marked this pull request as ready for review October 26, 2026 19:30
@greptile-apps

greptile-apps Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 3/5

[Medium impact] The PR adds shared utilities, with production-call-site changes limited to the Rerun CDR adapter.

Not safe to merge until the depth-resizing corruption and float-image reopening failure are fixed. The occupancy boundary concern is non-blocking.

Findings

  1. P1 Resizing changes depth values ▶
  2. P1 Saved float images cannot reopen ▶
  3. P2 Outside points read edge cells ▶

Summary

This PR groups generated-message helpers under domain modules and adds geometry, calibration, occupancy, image, and point-cloud operations.

  • Image helpers now offer safe pixel views and separate image-encoding outputs.
  • Point-cloud helpers build, select, transform, and color point records.
  • Geometry helpers convert and combine poses, rotations, and transforms.
  • Occupancy helpers load grids and map points between grid and world coordinates.
  • Camera calibration helpers build messages from intrinsics, field of view, or YAML.
  • Python callers use checked domain helpers for message operations.
  • Time helpers convert between ROS stamps and seconds or nanoseconds.

The affected helpers remain unchanged across all eight supplied higher-stack revisions through PR #4740; those PRs do not resolve these failures.

Reviews (1) · Last reviewed commit: "ci: remove dedicated CDR workflow" · Reviewed by Greptile

Comment on lines +317 to +320
pixels = cv2.resize(
view(message),
(max(1, int(message.width * scale)), max(1, int(message.height * scale))),
interpolation=cv2.INTER_LINEAR,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Resizing changes depth values

resize_to_fit passes big-endian pixels directly to OpenCV, which interprets their bytes in native order. A constant 16UC1 image containing 1000 becomes 59395 when resized from 2×2 to 1×1. The same corruption occurs when to_compressed resizes through max_width; PNG decoding then returns the incorrect depth.

Convert pixels to native byte order before resizing, as the PNG and JXL branches already do. This depth-data corruption should be fixed before merging.

TREX Ran code and verified through TREX

Comment on lines +159 to +163
if pixels.dtype.kind not in ("u", "f") or (
pixels.dtype.kind,
pixels.dtype.itemsize,
) not in (("u", 1), ("u", 2), ("f", 4)):
raise ValueError(f"JXL cannot encode dtype {pixels.dtype}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Saved float images cannot reopen

to_compressed accepts 32FC3 for JXL, but from_compressed labels every decoded three-channel image as rgb8. A float32 image therefore encodes successfully and then fails to reopen with ValueError: array dtype does not match rgb8, even though the JXL payload contains the correct float32 pixels.

Preserve 32FC3 when decoding float32 three-channel pixels, or reject that layout before encoding. The supported save-and-reopen flow should work before merging.

TREX Ran code and verified through TREX

def cell_value(message: OccupancyGrid, point: Point) -> int:
"""Read a world cell, retaining the planner's unknown result outside the map."""
x, y = world_to_grid(message, point)
column, row = int(x), int(y)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Outside points read edge cells

cell_value converts coordinates with int before checking bounds. Negative fractions round toward zero, so a point half a cell outside the map reads row or column zero instead of returning UNKNOWN. With an identity origin and resolution 1, both Point(x=-0.5, y=0, z=0) and the corresponding negative-y query return the edge cell's value.

This is a non-blocking boundary correctness concern: callers receive known free or occupied space for an outside-map point. Use math.floor before checking bounds, or check the continuous coordinates first.

Suggested change
column, row = int(x), int(y)
column, row = math.floor(x), math.floor(y)
Artifacts

Evidence from the check

  • The executed script constructs and CDR-round-trips real generated messages, tests boundary queries, and loads exact snapshot source without changing checkout, making the narrow reproduction repeatable.

Command output from the check

  • Ran 20 queries against the original helper with FREE and OCCUPIED grids, showing six negative-fraction failures while all 14 controls passed.

Command output from the check

  • Repeated the same 20 queries after replacing truncation with floor only in memory, showing that every case passed without modifying tracked source.

Command output from the check

  • Compared and executed the occupancy helper from all eight supplied exact SHAs, showing identical source and six failures per snapshot, so the higher stack does not fix this helper.

View artifacts

TREX Ran code and verified through TREX

@TomCC7
TomCC7 removed this pull request from stack #4845 October 26, 2026 20:35
@TomCC7
TomCC7 added this pull request to stack #4847 October 26, 2026 20:36

This branch had an error being deployed

1 failed (outdated) deployment
cachix — 65879496 Deployed Oct 23, 2026 by TomCC7 via cachix-build #14112
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.

1 participant