Skip to content

Visualiser proto contract parity: debug streaming, cluster fields, speed summary rename#334

Open
Copilot wants to merge 7 commits intomainfrom
copilot/complete-backlog-item-1
Open

Visualiser proto contract parity: debug streaming, cluster fields, speed summary rename#334
Copilot wants to merge 7 commits intomainfrom
copilot/complete-backlog-item-1

Conversation

Copy link
Contributor

Copilot AI commented Feb 27, 2026

Implements the remaining Go-side gaps in the visualiser protobuf contract before v0.5.0: FrameBundle.debug was declared but never serialised, cluster feature fields were dropped during proto conversion, and Track.avg_speed_mps carried mean semantics where median/percentile summaries are needed.

This is a breaking change for v0.5.x schema stabilisation — AvgSpeedMps is fully removed from the visualiser Track model with no VRLOG backward compatibility.

Proto schema

  • Rename field 24: avg_speed_mpsmedian_speed_mps
  • Add p85_speed_mps (field 36), p98_speed_mps (field 37)
  • Regenerated Go bindings via make proto-gen-go

Debug overlay serialisation

  • frameBundleToProto() now maps FrameBundle.Debugpb.DebugOverlaySet, gated by StreamRequest.include_debug
  • All four overlay types: association candidates, gating ellipses, innovation residuals, state predictions

Cluster field parity

  • Serialise height_p95, intensity_mean, sample_points (previously dropped in proto conversion)
  • flattenSamplePoints() converts [][3]float32 → flat []float32 for proto wire format
  • Propagated from l4perception.WorldCluster.SamplePoints through the adapter

Speed summary computation

  • speedPercentiles() computes median/p85/p98 from TrackedObject.SpeedHistory() using floor-index selection (sorts in-place — no extra copy since SpeedHistory() already returns a fresh slice)
  • Adapter populates new fields for both active and recently-deleted (fade-out) tracks
  • AvgSpeedMps fully removed from the visualiser Track model — MedianSpeedMps is the sole central speed measure
  • classifyOrConvert() uses MedianSpeedMps as the central speed input for replay classification (documented mapping)

Swift source updates

  • Renamed avgSpeedMpsmedianSpeedMps in Models.swift and VisualiserClient.swift to match proto field rename
  • Added p85SpeedMps and p98SpeedMps fields to Swift Track model
  • Updated all Swift test files referencing the renamed field (7 test files)
  • RunTrack.avgSpeedMps (REST API model) is unchanged — it maps to the storage layer's running average

Tests

  • Replaced negative TestFrameBundleToProto_DebugNotConverted (asserting Debug == nil) with positive round-trip assertions
  • Added: DebugSerialized, DebugOmittedWhenNilData, ClusterFeatureFields, SpeedSummaryFields, SpeedPercentiles_*, FlattenSamplePoints_*, SortsInPlace
  • All classification tests updated to use MedianSpeedMps

Not in scope

Swift binding regeneration (Phase D) requires macOS CI — proto stubs are generated at CI time via make proto-gen-swift.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Implement visualiser proto contract parity and debug overlays Visualiser proto contract parity: debug streaming, cluster fields, speed summary rename Feb 28, 2026
@ddol ddol marked this pull request as ready for review February 28, 2026 00:21
Copilot AI review requested due to automatic review settings February 28, 2026 00:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Closes remaining Go-side gaps between the LiDAR visualiser’s internal model and the protobuf streaming contract ahead of v0.5.0, covering debug overlay streaming, cluster feature parity, and updated track speed summary semantics (median + percentiles).

Changes:

  • Updates Track proto schema to rename speed field 24 to median_speed_mps and adds p85_speed_mps / p98_speed_mps.
  • Extends Go adapter + gRPC serializer to propagate cluster feature fields (height_p95, intensity_mean, sample_points) and debug overlays (gated by include_debug).
  • Adds/updates Go tests to assert positive end-to-end serialisation for debug overlays, cluster features, and new speed summary fields/percentile helper behaviour.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
proto/velocity_visualiser/v1/visualiser.proto Renames track speed field to median semantics and adds p85/p98 percentile fields.
internal/lidar/visualiser/pb/visualiser_grpc.pb.go Regenerated gRPC bindings reflecting updated proto.
internal/lidar/visualiser/pb/visualiser.pb.go Regenerated Go proto bindings reflecting renamed/new fields.
internal/lidar/visualiser/object_class_conversion_test.go Updates tests for renamed MedianSpeedMps field usage.
internal/lidar/visualiser/model_test.go Updates model test fixtures for MedianSpeedMps.
internal/lidar/visualiser/model.go Updates canonical Track model to include median/p85/p98 fields.
internal/lidar/visualiser/grpc_server_test.go Adds positive tests for debug overlay serialisation and new cluster/track fields.
internal/lidar/visualiser/grpc_server.go Serialises debug overlays + cluster feature fields + new speed summary fields into proto.
internal/lidar/visualiser/adapter_test.go Adds unit tests for speedPercentiles() and flattenSamplePoints().
internal/lidar/visualiser/adapter.go Computes median/p85/p98 from speed history; propagates flattened sample points.
docs/plans/lidar-visualiser-proto-contract-and-debug-overlay-fixes-plan.md Marks Go-side phases complete and updates checklist/status.

@codecov
Copy link

codecov bot commented Feb 28, 2026

Codecov Report

❌ Patch coverage is 97.72727% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/lidar/visualiser/adapter.go 92.85% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Copilot AI and others added 4 commits February 27, 2026 17:06
… and speed summary rename

Co-authored-by: ddol <445312+ddol@users.noreply.github.com>
Co-authored-by: ddol <445312+ddol@users.noreply.github.com>
Co-authored-by: ddol <445312+ddol@users.noreply.github.com>
@ddol ddol force-pushed the copilot/complete-backlog-item-1 branch from e427ee8 to 8e23c4c Compare February 28, 2026 01:06
@ddol
Copy link
Member

ddol commented Feb 28, 2026

@copilot apply changes based on the comments in this thread fix ci

…compat and classifier, fix British spelling, eliminate double-copy, update Swift source for proto rename

Co-authored-by: ddol <445312+ddol@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 28, 2026

@copilot apply changes based on the comments in this thread fix ci

All review comments addressed and the macOS CI failure fixed in 59689c9:

  1. Restored AvgSpeedMps for VRLOG backward compat and classifier
  2. classifyOrConvert() uses AvgSpeedMps (mean) not median
  3. British English: serialisation
  4. Eliminated double-copy in speedPercentiles()
  5. Updated Swift source (Models.swift, VisualiserClient.swift, 7 test files) for proto field rename avgSpeedMpsmedianSpeedMps + new p85SpeedMps/p98SpeedMps

@github-actions
Copy link

Coverage report

This PR does not seem to contain any modification to coverable code.

@ddol
Copy link
Member

ddol commented Feb 28, 2026

@copilot fully remove average (mean) fields, no legacy-back compat. this is a breaking change to stabilise the schema for v0.5.x. ensure 98%+ coverage for lines updated in this pr

Copilot AI and others added 2 commits February 28, 2026 01:40
…ange for v0.5.x schema stabilisation

Co-authored-by: ddol <445312+ddol@users.noreply.github.com>
Co-authored-by: ddol <445312+ddol@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 28, 2026

@copilot fully remove average (mean) fields, no legacy-back compat. this is a breaking change to stabilise the schema for v0.5.x. ensure 98%+ coverage for lines updated in this pr

Removed AvgSpeedMps from the visualiser Track model, adapter, and all test references. classifyOrConvert() now uses MedianSpeedMps as the central speed input (documented). Coverage for changed lines: classifyOrConvert 100%, frameBundleToProto 96.2%, adaptTracks 96.9%, adaptClusters 100%, flattenSamplePoints 100%, adaptDebugFrame 100%. The only sub-100% function is speedPercentiles at 90.9% due to an unreachable idx >= n safety guard. Fixed in b617bc4.

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.

3 participants