Add color support#76
Conversation
- Provides Color(), ColorAt(), SetColor(), and standard iterator methods (Incr, IsValid, RawIndex) on packed RGB/RGBA point cloud fields. PointCloud.ColorIterator() auto-detects "rgb" or "rgba". - Add tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #76 +/- ##
==========================================
+ Coverage 86.24% 88.15% +1.91%
==========================================
Files 33 34 +1
Lines 1941 1706 -235
==========================================
- Hits 1674 1504 -170
+ Misses 208 143 -65
Partials 59 59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds first-class packed RGB/RGBA color handling to the pc point cloud data model via a new Color type and iterator support, with tests validating iterator behavior and I/O round-trips.
Changes:
- Introduce
Colortype with PCL-compatible packing/unpacking (0xAARRGGBB) and float32/uint32 conversions. - Add
ColorIteratorAPI toPointCloudplus a backingcolorIteratorimplementation. - Add unit tests for color packing, iterators (rgb/rgba), and marshal/unmarshal round-trips.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pc/randomaccess.go | No functional change (formatting/newline). |
| pc/pointcloud.go | Adds PointCloud.ColorIterator() that locates rgb/rgba fields and returns a color iterator. |
| pc/iterator.go | Adds Color*Iterator interfaces and colorIterator implementation over packed 4-byte fields. |
| pc/iterator_test.go | Adds iterator-level tests for ColorIterator across rgb/rgba/no-field cases. |
| pc/io_test.go | Adds round-trip marshal/unmarshal test verifying color bytes are preserved. |
| pc/color.go | New Color type and conversion helpers matching PCL packed layout. |
| pc/color_test.go | Adds tests for Color float32/uint32 round-trip and bit layout expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, c := range testCases { | ||
| t.Run("Uint32", func(t *testing.T) { | ||
| v := c.Uint32() | ||
| got := ColorFromUint32(v) | ||
| if got != c { | ||
| t.Errorf("Uint32 round-trip: expected %v, got %v", c, got) | ||
| } | ||
| }) | ||
| t.Run("Float32", func(t *testing.T) { | ||
| f := c.Float32() | ||
| got := ColorFromFloat32(f) | ||
| if got != c { | ||
| t.Errorf("Float32 round-trip: expected %v, got %v", c, got) | ||
| } | ||
| }) |
There was a problem hiding this comment.
In this loop you pass a closure to t.Run that closes over the range variable c. While it happens to work as written (no t.Parallel), the repo targets Go 1.16 and other tests consistently rebind loop variables before t.Run (e.g., tt := tt in pc/iterator_test.go) to avoid future capture bugs. Consider rebinding c := c inside the loop (or use table-driven subtests with unique names) before starting subtests.
No description provided.