From de036c9b7158e0ae23908e94430fde1a3dcb079e Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sat, 14 Nov 2026 09:18:57 +0100 Subject: [PATCH 1/2] fix(builtin): decoders skip unknown fields and read absent fields as zero (1.0.0) Three wire decisions that a 1.0.0 host would otherwise have frozen. Unknown fields. decodePointField read a varint before it knew the field, then reported "unknown", and parseFields skipped a second field. A field added to PointField by a later SDK would have broken PointCloud, GridMap and VoxelGrid decoding on every 1.0.0 host. parseFields now has one rule: false with nothing read means unknown and is skipped; false after reading means a known field is malformed and the decode fails. decodePointField checks the field number before it reads. Absent fields. A standard proto3 encoder omits a zero, so an absent field is zero. Decoders that started from the C++ struct defaults turned is_dense = false into true, height 0 into 1, PointField.count 0 into 1, a cone into a cylinder, and a transparent annotation colour into an opaque one. They now start from the proto3 zero values; the empty PointCloud payload agrees. Bytes written by the SDK's own serializers are unaffected. A NaN colour channel decodes as 0 instead of reaching an undefined cast. Log.category moves from field 5 to 7, with 5 and 6 reserved: they are file and line in foxglove.Log, so a Foxglove log put its file path in category. Only 1.0.0-dev.1 shipped field 5. Co-Authored-By: Claude Fable 5.2 --- CHANGELOG.md | 27 +++++- docs/builtin_type.md | 15 +++ docs/image_annotations_format.md | 2 + pj_base/proto/pj/Log.proto | 6 +- pj_base/proto/pj/PointCloud.proto | 2 +- pj_base/src/builtin/builtin_object_codec.cpp | 8 ++ pj_base/src/builtin/geometry_codec.hpp | 3 +- .../src/builtin/image_annotations_codec.cpp | 49 +--------- pj_base/src/builtin/log_codec.cpp | 4 +- pj_base/src/builtin/plot_markers_codec.cpp | 3 +- pj_base/src/builtin/point_cloud_codec.cpp | 3 + pj_base/src/builtin/point_field_codec.hpp | 5 +- pj_base/src/builtin/protobuf_wire.hpp | 19 +++- pj_base/src/builtin/scene_entities_codec.cpp | 4 + pj_base/tests/builtin_object_codec_test.cpp | 12 +++ .../tests/image_annotations_codec_test.cpp | 47 ++++++++++ pj_base/tests/log_codec_test.cpp | 25 ++++- pj_base/tests/point_cloud_codec_test.cpp | 91 +++++++++++++++++++ pj_base/tests/scene_entities_codec_test.cpp | 23 +++++ 19 files changed, 282 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52a00154..23f4d46a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,9 +32,11 @@ Functional parser modules leave the installed SDK: Log: -- `PJ::sdk::Log` gains an optional `category` (proto field 5 of `PJ.Log`): a source-defined grouping +- `PJ::sdk::Log` gains an optional `category` (proto field 7 of `PJ.Log`): a source-defined grouping (a tag, a subsystem) a log viewer can show and filter on. The codec writes it only when non-empty; - readers built before this release skip the unknown field. + readers built before this release skip the unknown field. Fields 5 and 6 are reserved: they are + `file` and `line` in `foxglove.Log`, so a Foxglove-encoded log decodes with both skipped. + (`1.0.0-dev.1` wrote `category` as field 5; a log encoded by a `dev.1` build loses its category.) - **Breaking (source):** `PJ::sdk::Log::name` is renamed `source`; proto field 4 keeps its number, so the wire format and binaries built against 0.x are unaffected. Code that reads or sets the field must be updated. This rename is why the release is 1.0.0. @@ -151,6 +153,27 @@ The direct C++ parser bridge (docs only, no code change): (`frame_id`). A parser built against 0.x that returns either type must be rebuilt for a 1.0 host. No parser in pj-official-plugins returns them. +Builtin decoders: + +- An unknown varint field inside a `PJ.PointField` no longer breaks the decode of the `PointCloud`, + `GridMap` or `VoxelGrid` that holds it. The decoder read the value and then skipped a second + field, so a field added to `PointField` by a later SDK would have been unreadable by every 1.0.0 + host. The shared field loop now has one rule for every codec: an unknown field is skipped, a known + field that is malformed fails the decode. Before, a malformed nested message could skip the bytes + that followed it and decode as something else. +- **Behaviour (wire):** fields absent from the wire now decode as the proto3 zero value where the + C++ struct default differs. A standard protobuf encoder omits a zero, so `PointCloud.is_dense = + false` decoded as `true`, `PointCloud.height = 0` as `1`, `PointField.count = 0` as `1`, and a cone + (`CylinderPrimitive.top_scale = 0`) as a cylinder. Bytes written by the SDK's own serializers are + unaffected: they write every one of these fields. `GridMap` rejects a `PointField` whose `count` is + absent, as it already rejected an explicit `0`. The empty `PointCloud` payload decodes to the same + zero values. +- **Behaviour (wire):** a colour inside an `ImageAnnotations` message decodes like every other + `PJ.Color`: a channel absent from a colour that is present is 0. Alpha used to default to 1 there, + so a "no fill" colour written as `{r, g, b, a = 0}` by a standard encoder came back opaque. A + colour message that is absent altogether keeps its documented default. +- A NaN colour channel decodes as 0; it used to reach an undefined float-to-integer conversion. + ## [0.40.0] Host contract: extended: pj.mcp_endpoint.v1, PJ_mcp_endpoint_host_vtable_t::acquire_endpoint, diff --git a/docs/builtin_type.md b/docs/builtin_type.md index b6216de9..afceee8d 100644 --- a/docs/builtin_type.md +++ b/docs/builtin_type.md @@ -128,6 +128,21 @@ current implementation does not require generated Protobuf code or a Protobuf runtime dependency; it uses private field-tagged wire primitives and maps only between bytes and SDK structs. +Two decode rules hold for every codec, so that bytes from a standard Protobuf +encoder and from a later SDK both read correctly: + +- **An absent field is the proto3 zero value**, even where the C++ struct + default differs (`PointCloud::is_dense`, `PointCloud::height`, + `PointField::count`, `CylinderPrimitive::bottom_scale` / `top_scale`, a colour + channel). A standard encoder omits a zero, so a decoder that kept the struct + default would turn `is_dense = false` into `true`. The SDK's own serializers + write these fields explicitly. A whole submessage that is absent may keep a + documented default (an annotation's colour, for example). +- **An unknown field is skipped; a known field that is malformed fails the + decode.** A field added by a later SDK must be readable by an older one. A + field handler therefore decides whether it knows a field before it reads any + byte of it. + ## Type Erasure and Classification `BuiltinObjectType` is the a-priori tag a parser reports for a schema. It lets diff --git a/docs/image_annotations_format.md b/docs/image_annotations_format.md index 0e121f8a..458630ac 100644 --- a/docs/image_annotations_format.md +++ b/docs/image_annotations_format.md @@ -84,6 +84,8 @@ Circles are stored in the wire schema as `diameter`; the SDK stores `radius`. The codec converts between `diameter` and `radius * 2.0`. Colors are stored in the schema as normalized `double` channels in `[0, 1]`. +A channel absent from a colour that is present is 0, alpha included; a NaN +channel decodes as 0. The SDK stores RGBA `uint8_t` channels. Decode clamps normalized values to `[0, 1]` and rounds to the nearest byte, so a round trip may differ by one channel value because of floating-point rounding. diff --git a/pj_base/proto/pj/Log.proto b/pj_base/proto/pj/Log.proto index 45e5188e..e4fc0cf0 100644 --- a/pj_base/proto/pj/Log.proto +++ b/pj_base/proto/pj/Log.proto @@ -37,6 +37,10 @@ message Log { // Who emitted it: a process, node or logger name. string source = 4; + // 5 and 6 are `file` and `line` in foxglove.Log. They stay unused here so a + // Foxglove-encoded log decodes with those two fields skipped. + reserved 5, 6; + // Source-defined grouping of the message (a tag, a subsystem); may be empty. - string category = 5; + string category = 7; } diff --git a/pj_base/proto/pj/PointCloud.proto b/pj_base/proto/pj/PointCloud.proto index af1c0c28..ac3892ed 100644 --- a/pj_base/proto/pj/PointCloud.proto +++ b/pj_base/proto/pj/PointCloud.proto @@ -53,7 +53,7 @@ message PointField { // Scalar data type of one element of this channel Datatype datatype = 3; - // Number of elements of `datatype` per point (typically 1) + // Number of elements of `datatype` per point (typically 1). Absent means 0, which is not a usable channel: write it. uint32 count = 4; } diff --git a/pj_base/src/builtin/builtin_object_codec.cpp b/pj_base/src/builtin/builtin_object_codec.cpp index b3fbd0a3..22a333ae 100644 --- a/pj_base/src/builtin/builtin_object_codec.cpp +++ b/pj_base/src/builtin/builtin_object_codec.cpp @@ -117,6 +117,14 @@ Expected deserializeBuiltinObject(sdk::BuiltinObjectType typ case sdk::BuiltinObjectType::kImage: return deserializeAs(data, size, deserializeImage); case sdk::BuiltinObjectType::kPointCloud: + if (size == 0) { + // The empty message holds the proto3 zero values, which are not + // PointCloud's struct defaults (height 1, is_dense true). + sdk::PointCloud cloud; + cloud.height = 0; + cloud.is_dense = false; + return sdk::BuiltinObject(std::move(cloud)); + } return deserializeAs(data, size, deserializePointCloud); case sdk::BuiltinObjectType::kDepthImage: return deserializeAs(data, size, deserializeDepthImage); diff --git a/pj_base/src/builtin/geometry_codec.hpp b/pj_base/src/builtin/geometry_codec.hpp index 99d7eef4..a182cee9 100644 --- a/pj_base/src/builtin/geometry_codec.hpp +++ b/pj_base/src/builtin/geometry_codec.hpp @@ -16,6 +16,7 @@ // Inline-only; not exposed through the public include path. #include +#include #include #include "pj_base/builtin/frame_transforms.hpp" // Vector2, Vector3, Quaternion, Pose @@ -251,7 +252,7 @@ inline bool readPoseMessage(Reader& reader, sdk::Pose& out) { // ---------- Color (uint8 RGBA <-> double 0..1) ---------- inline uint8_t normalizedToByte(double value) { - value = std::clamp(value, 0.0, 1.0); + value = std::isnan(value) ? 0.0 : std::clamp(value, 0.0, 1.0); // a NaN would make the cast undefined return static_cast(value * 255.0 + 0.5); } diff --git a/pj_base/src/builtin/image_annotations_codec.cpp b/pj_base/src/builtin/image_annotations_codec.cpp index 5cf94848..f3142987 100644 --- a/pj_base/src/builtin/image_annotations_codec.cpp +++ b/pj_base/src/builtin/image_annotations_codec.cpp @@ -14,7 +14,6 @@ namespace PJ { namespace { -using builtin_wire::normalizedToByte; using builtin_wire::Reader; using builtin_wire::Tag; using builtin_wire::WireType; @@ -117,57 +116,15 @@ bool decodePoint2(Reader& reader, Point2& out) { return true; } -// Not builtin_wire::decodeColor: annotation colours default alpha to 1 and skip unknown fields. -bool decodeAnnotationColor(Reader& reader, ColorRGBA& out) { - double r = 0.0; - double g = 0.0; - double b = 0.0; - double a = 1.0; - - while (!reader.eof()) { - Tag tag; - if (!reader.readTag(tag)) { - return false; - } - - if (tag.type == WireType::kFixed64 && tag.field >= 1 && tag.field <= 4) { - double value = 0.0; - if (!reader.readDouble(value)) { - return false; - } - switch (tag.field) { - case 1: - r = value; - break; - case 2: - g = value; - break; - case 3: - b = value; - break; - case 4: - a = value; - break; - default: - break; - } - } else if (!reader.skip(tag.type)) { - return false; - } - } - - out = {normalizedToByte(r), normalizedToByte(g), normalizedToByte(b), normalizedToByte(a)}; - return true; -} - bool readPoint2Message(Reader& reader, Point2& out) { Reader nested; return reader.readMessage(nested) && decodePoint2(nested, out); } +// A colour that is present decodes like every other PJ.Color: an omitted +// channel is 0, alpha included, because a standard encoder omits a zero. bool readAnnotationColorMessage(Reader& reader, ColorRGBA& out) { - Reader nested; - return reader.readMessage(nested) && decodeAnnotationColor(nested, out); + return builtin_wire::readColorMessage(reader, out); } bool decodePointsAnnotation(Reader& reader, PointsAnnotation& out) { diff --git a/pj_base/src/builtin/log_codec.cpp b/pj_base/src/builtin/log_codec.cpp index 6a7e1505..077800e4 100644 --- a/pj_base/src/builtin/log_codec.cpp +++ b/pj_base/src/builtin/log_codec.cpp @@ -48,7 +48,7 @@ std::vector serializeLog(const Log& log) { writer.string(3, log.message); writer.string(4, log.source); if (!log.category.empty()) { - writer.string(5, log.category); + writer.string(7, log.category); } return out; @@ -81,7 +81,7 @@ Expected deserializeLog(const uint8_t* data, size_t size) { return tag.type == WireType::kLengthDelimited && r.readString(log.message); case 4: return tag.type == WireType::kLengthDelimited && r.readString(log.source); - case 5: + case 7: return tag.type == WireType::kLengthDelimited && r.readString(log.category); default: return false; diff --git a/pj_base/src/builtin/plot_markers_codec.cpp b/pj_base/src/builtin/plot_markers_codec.cpp index 054f5565..f2d77b70 100644 --- a/pj_base/src/builtin/plot_markers_codec.cpp +++ b/pj_base/src/builtin/plot_markers_codec.cpp @@ -4,6 +4,7 @@ #include "pj_base/builtin/plot_markers_codec.hpp" #include +#include #include #include #include @@ -87,7 +88,7 @@ MarkerSeverity mapSeverity(uint64_t v) { } uint8_t normalizedToByte(double value) { - value = std::clamp(value, 0.0, 1.0); + value = std::isnan(value) ? 0.0 : std::clamp(value, 0.0, 1.0); // a NaN would make the cast undefined return static_cast(value * 255.0 + 0.5); } diff --git a/pj_base/src/builtin/point_cloud_codec.cpp b/pj_base/src/builtin/point_cloud_codec.cpp index b5b04395..86e34509 100644 --- a/pj_base/src/builtin/point_cloud_codec.cpp +++ b/pj_base/src/builtin/point_cloud_codec.cpp @@ -77,6 +77,9 @@ Expected decodePointCloud(const uint8_t* data, size_t size, con Reader reader(data, size); sdk::PointCloud cloud; + // The proto3 zero values, which a standard encoder omits; the struct defaults differ. + cloud.height = 0; + cloud.is_dense = false; const bool ok = parseFields(reader, [&](Tag tag, Reader& r) { switch (tag.field) { diff --git a/pj_base/src/builtin/point_field_codec.hpp b/pj_base/src/builtin/point_field_codec.hpp index cab4bcc4..c3c10b02 100644 --- a/pj_base/src/builtin/point_field_codec.hpp +++ b/pj_base/src/builtin/point_field_codec.hpp @@ -59,12 +59,13 @@ inline void writePointField(Writer& writer, const sdk::PointField& field) { } inline bool decodePointField(Reader& reader, sdk::PointField& out) { + out.count = 0; // the proto3 zero value, omitted by encoders; the struct default is 1 return parseFields(reader, [&](Tag tag, Reader& r) { if (tag.field == 1) { return tag.type == WireType::kLengthDelimited && r.readString(out.name); } - if (tag.type != WireType::kVarint) { - return false; + if (tag.field < 2 || tag.field > 4 || tag.type != WireType::kVarint) { + return false; // unknown, or an unexpected wire type: skipped, nothing read yet } uint64_t v = 0; if (!r.readVarint(v)) { diff --git a/pj_base/src/builtin/protobuf_wire.hpp b/pj_base/src/builtin/protobuf_wire.hpp index 3562eb94..635aadf6 100644 --- a/pj_base/src/builtin/protobuf_wire.hpp +++ b/pj_base/src/builtin/protobuf_wire.hpp @@ -322,10 +322,15 @@ class Reader { }; /// Drives the standard tag-read / field-dispatch / skip-unknown loop used -/// by every per-message decoder. `handler(tag, reader)` consumes the field -/// and returns true when handled (including its own wire-type validation), -/// false to let the loop skip the field via reader.skip(). Returns false on -/// truncation / malformed input. +/// by every per-message decoder. `handler(tag, reader)` returns: +/// - true: it consumed the field; +/// - false, having read nothing: the field is unknown to it (or has a wire +/// type it does not expect), and the loop skips it. This is what keeps a +/// field added by a later SDK readable, so a handler decides whether it +/// knows the field BEFORE it reads; +/// - false, having read something: a known field is malformed, and the +/// decode fails. Skipping here would skip a second field. +/// Returns false on truncation / malformed input. template [[nodiscard]] inline bool parseFields(Reader& reader, FieldHandler&& handler) { while (!reader.eof()) { @@ -333,7 +338,11 @@ template if (!reader.readTag(tag)) { return false; } - if (!handler(tag, reader) && !reader.skip(tag.type)) { + const size_t before = reader.remaining(); + if (handler(tag, reader)) { + continue; + } + if (reader.remaining() != before || !reader.skip(tag.type)) { return false; } } diff --git a/pj_base/src/builtin/scene_entities_codec.cpp b/pj_base/src/builtin/scene_entities_codec.cpp index dc547056..172d0cf1 100644 --- a/pj_base/src/builtin/scene_entities_codec.cpp +++ b/pj_base/src/builtin/scene_entities_codec.cpp @@ -179,6 +179,10 @@ void writeCylinderPrimitive(Writer& writer, const CylinderPrimitive& p) { } bool decodeCylinderPrimitive(Reader& reader, CylinderPrimitive& out) { + // The proto3 zero values, which a standard encoder omits: a cone has + // top_scale 0. The struct defaults are 1. + out.bottom_scale = 0.0; + out.top_scale = 0.0; return parseFields(reader, [&](Tag tag, Reader& r) { switch (tag.field) { case 1: diff --git a/pj_base/tests/builtin_object_codec_test.cpp b/pj_base/tests/builtin_object_codec_test.cpp index f2f22ce1..468c1394 100644 --- a/pj_base/tests/builtin_object_codec_test.cpp +++ b/pj_base/tests/builtin_object_codec_test.cpp @@ -143,4 +143,16 @@ TEST(BuiltinObjectCodec, LargePayloadIsNotCopiedTwice) { EXPECT_LT(encoded->capacity(), points.size() + 4096); } +// The empty message is every field at its proto3 zero value. For PointCloud +// those are not the struct defaults, and the empty payload must agree with a +// payload that carries any one field. +TEST(BuiltinObjectCodec, EmptyPointCloudPayloadHoldsTheProto3ZeroValues) { + const auto decoded = PJ::deserializeBuiltinObject(PJ::sdk::BuiltinObjectType::kPointCloud, nullptr, 0); + ASSERT_TRUE(decoded) << decoded.error(); + const auto* cloud = decoded->get(); + ASSERT_NE(cloud, nullptr); + EXPECT_EQ(cloud->height, 0u); + EXPECT_FALSE(cloud->is_dense); +} + } // namespace diff --git a/pj_base/tests/image_annotations_codec_test.cpp b/pj_base/tests/image_annotations_codec_test.cpp index 76d7f345..9057767b 100644 --- a/pj_base/tests/image_annotations_codec_test.cpp +++ b/pj_base/tests/image_annotations_codec_test.cpp @@ -412,5 +412,52 @@ TEST(ImageAnnotationCodecTest, TimestampOutsideInt64NanosIsRejected) { EXPECT_FALSE(decodes(9223372037, 0)); } +// A colour that is present with its alpha omitted is transparent: a standard +// proto3 encoder omits a zero channel. A "no fill" colour written as +// {r, g, b, a = 0} must not come back as an opaque fill. +TEST(ImageAnnotationCodecTest, OmittedAlphaInAPresentColorIsTransparent) { + std::vector red_no_alpha; + pb::appendTag(red_no_alpha, 1, 1); + pb::appendDouble(red_no_alpha, 1.0); + + std::vector circle; + pb::appendTag(circle, 5, 2); // fill_color + pb::appendLenDelim(circle, red_no_alpha); + pb::appendTag(circle, 6, 2); // color: present and empty, so every channel is 0 + pb::appendLenDelim(circle, {}); + + std::vector bytes; + pb::appendTag(bytes, 1, 2); + pb::appendLenDelim(bytes, circle); + + auto out = deserializeImageAnnotations(bytes.data(), bytes.size()); + ASSERT_TRUE(out.has_value()) << out.error(); + ASSERT_EQ(out->circles.size(), 1u); + EXPECT_EQ(out->circles[0].fill_color, (sdk::ColorRGBA{255, 0, 0, 0})); + EXPECT_EQ(out->circles[0].color, (sdk::ColorRGBA{0, 0, 0, 0})); +} + +// A NaN channel decodes as 0 instead of reaching an undefined float-to-integer cast. +TEST(ImageAnnotationCodecTest, NaNColorChannelDecodesAsZero) { + std::vector color; + pb::appendTag(color, 1, 1); + pb::appendDouble(color, std::numeric_limits::quiet_NaN()); + pb::appendTag(color, 4, 1); + pb::appendDouble(color, 1.0); + + std::vector circle; + pb::appendTag(circle, 6, 2); + pb::appendLenDelim(circle, color); + + std::vector bytes; + pb::appendTag(bytes, 1, 2); + pb::appendLenDelim(bytes, circle); + + auto out = deserializeImageAnnotations(bytes.data(), bytes.size()); + ASSERT_TRUE(out.has_value()) << out.error(); + ASSERT_EQ(out->circles.size(), 1u); + EXPECT_EQ(out->circles[0].color, (sdk::ColorRGBA{0, 0, 0, 255})); +} + } // namespace } // namespace PJ diff --git a/pj_base/tests/log_codec_test.cpp b/pj_base/tests/log_codec_test.cpp index 45dbad4f..41a1cb55 100644 --- a/pj_base/tests/log_codec_test.cpp +++ b/pj_base/tests/log_codec_test.cpp @@ -73,7 +73,7 @@ TEST(LogCodecTest, EmptyCategoryIsNotWritten) { const auto bytes = serializeLog(in); for (uint8_t b : bytes) { - EXPECT_NE(b, 0x2A); // field 5, length-delimited + EXPECT_NE(b, 0x3A); // field 7, length-delimited } auto out = deserializeLog(bytes.data(), bytes.size()); ASSERT_TRUE(out.has_value()); @@ -82,8 +82,8 @@ TEST(LogCodecTest, EmptyCategoryIsNotWritten) { } TEST(LogCodecTest, UnknownFieldIsSkipped) { - // field 8 (varint) = 1, then field 3 "hi", field 5 "net". - const std::vector bytes = {0x40, 0x01, 0x1A, 0x02, 'h', 'i', 0x2A, 0x03, 'n', 'e', 't'}; + // field 8 (varint) = 1, then field 3 "hi", field 7 "net". + const std::vector bytes = {0x40, 0x01, 0x1A, 0x02, 'h', 'i', 0x3A, 0x03, 'n', 'e', 't'}; auto out = deserializeLog(bytes.data(), bytes.size()); ASSERT_TRUE(out.has_value()); EXPECT_EQ(out->message, "hi"); @@ -91,14 +91,29 @@ TEST(LogCodecTest, UnknownFieldIsSkipped) { } TEST(LogCodecTest, WrongWireTypeForCategoryIsSkippedLikeAnyKnownField) { - // Field 5 sent as a varint: the handler declines it and parseFields skips it, + // Field 7 sent as a varint: the handler declines it and parseFields skips it, // as it does for every other known field. category stays empty. - const std::vector bytes = {0x1A, 0x02, 'h', 'i', 0x28, 0x07}; + const std::vector bytes = {0x1A, 0x02, 'h', 'i', 0x38, 0x07}; auto out = deserializeLog(bytes.data(), bytes.size()); ASSERT_TRUE(out.has_value()); EXPECT_EQ(out->message, "hi"); EXPECT_TRUE(out->category.empty()); } +// foxglove.Log uses field 5 for `file` (string) and field 6 for `line` +// (fixed32). PJ.Log omits both, so a Foxglove-encoded log decodes with them +// skipped: the file path must not turn up as the category. +TEST(LogCodecTest, FoxgloveFileAndLineAreNotTheCategory) { + const std::vector bytes = {0x1A, 0x02, 'h', 'i', // message = "hi" + 0x2A, 0x03, 'a', '.', 'c', // file = "a.c" + 0x35, 0x2A, 0x00, 0x00, 0x00, // line = 42 + 0x22, 0x01, 'n'}; // name / source = "n" + auto out = deserializeLog(bytes.data(), bytes.size()); + ASSERT_TRUE(out.has_value()); + EXPECT_EQ(out->message, "hi"); + EXPECT_EQ(out->source, "n"); + EXPECT_TRUE(out->category.empty()); +} + } // namespace } // namespace PJ diff --git a/pj_base/tests/point_cloud_codec_test.cpp b/pj_base/tests/point_cloud_codec_test.cpp index 738cb358..adff1fac 100644 --- a/pj_base/tests/point_cloud_codec_test.cpp +++ b/pj_base/tests/point_cloud_codec_test.cpp @@ -67,6 +67,97 @@ TEST(PointCloudCodecTest, RoundTripXYZIntensity) { EXPECT_EQ(std::memcmp(out->data.data(), payload.data(), payload.size()), 0); } +// A PointField body: name, then `extra` (bytes spliced in verbatim), then offset / datatype / count. +std::vector pointFieldBody(const std::vector& extra) { + std::vector body; + pb::appendTag(body, 1, 2); + pb::appendString(body, "x"); + body.insert(body.end(), extra.begin(), extra.end()); + pb::appendTag(body, 2, 0); + pb::appendVarint(body, 4); + pb::appendTag(body, 3, 0); + pb::appendVarint(body, 7); // FLOAT32 + pb::appendTag(body, 4, 0); + pb::appendVarint(body, 3); + return body; +} + +// A PointCloud holding one PointField (field 8) followed by frame_id (field 10). +std::vector cloudWithField(const std::vector& field_body) { + std::vector bytes; + pb::appendTag(bytes, 8, 2); + pb::appendLenDelim(bytes, field_body); + pb::appendTag(bytes, 10, 2); + pb::appendString(bytes, "lidar"); + return bytes; +} + +// A later SDK may add a field of any wire type to PointField. This decoder has +// to skip it and stay in sync: the fields after it, and the cloud's own fields +// after the PointField, must still decode. +TEST(PointCloudCodecTest, UnknownFieldsInsideAPointFieldAreSkipped) { + std::vector varint_field; + pb::appendTag(varint_field, 9, 0); + pb::appendVarint(varint_field, 300); + + std::vector fixed64_field; + pb::appendTag(fixed64_field, 10, 1); + pb::appendFixed64(fixed64_field, 0x0102030405060708ULL); + + std::vector bytes_field; + pb::appendTag(bytes_field, 11, 2); + pb::appendString(bytes_field, "later"); + + std::vector fixed32_field; + pb::appendTag(fixed32_field, 12, 5); + pb::appendFixed32(fixed32_field, 0x01020304U); + + for (const auto& extra : {varint_field, fixed64_field, bytes_field, fixed32_field}) { + const auto bytes = cloudWithField(pointFieldBody(extra)); + auto out = deserializePointCloud(bytes.data(), bytes.size()); + ASSERT_TRUE(out.has_value()) << "unknown field with first byte " << int{extra.front()} << ": " << out.error(); + ASSERT_EQ(out->fields.size(), 1u); + EXPECT_EQ(out->fields[0].name, "x"); + EXPECT_EQ(out->fields[0].offset, 4u); + EXPECT_EQ(out->fields[0].datatype, PointField::Datatype::kFloat32); + EXPECT_EQ(out->fields[0].count, 3u); + EXPECT_EQ(out->frame_id, "lidar"); + } +} + +// A known field that is malformed fails the decode. The bytes after the bad +// PointField are chosen so that skipping a second field over them would land +// exactly on the end of the buffer and report success. +TEST(PointCloudCodecTest, MalformedPointFieldFailsTheDecode) { + std::vector bytes; + pb::appendTag(bytes, 8, 2); + pb::appendLenDelim(bytes, {0x10, 0x80}); // offset: a varint cut short + bytes.insert(bytes.end(), {0x02, 0x10, 0x07}); + EXPECT_FALSE(deserializePointCloud(bytes.data(), bytes.size()).has_value()); +} + +// A standard proto3 encoder omits every field equal to its zero value, so a +// missing field means zero, whatever the C++ struct's own default is. +TEST(PointCloudCodecTest, AbsentFieldsDecodeAsTheProto3ZeroValues) { + std::vector name_only; + pb::appendTag(name_only, 1, 2); + pb::appendString(name_only, "x"); + + std::vector bytes; + pb::appendTag(bytes, 2, 0); + pb::appendVarint(bytes, 3); // width + pb::appendTag(bytes, 8, 2); + pb::appendLenDelim(bytes, name_only); + + auto out = deserializePointCloud(bytes.data(), bytes.size()); + ASSERT_TRUE(out.has_value()) << out.error(); + EXPECT_EQ(out->width, 3u); + EXPECT_EQ(out->height, 0u); + EXPECT_FALSE(out->is_dense); + ASSERT_EQ(out->fields.size(), 1u); + EXPECT_EQ(out->fields[0].count, 0u); +} + TEST(PointCloudCodecTest, FrameIdAbsentRoundTrips) { PointCloud in; in.width = 1; diff --git a/pj_base/tests/scene_entities_codec_test.cpp b/pj_base/tests/scene_entities_codec_test.cpp index 15d7a1f4..10482708 100644 --- a/pj_base/tests/scene_entities_codec_test.cpp +++ b/pj_base/tests/scene_entities_codec_test.cpp @@ -344,5 +344,28 @@ TEST(SceneEntitiesCodecTest, DeletionsOnlyBatchRoundTrips) { EXPECT_EQ(out->deletions[0].type, SceneEntityDeletion::Type::kAll); } +// A cone is a cylinder whose top_scale is 0. A standard proto3 encoder omits a +// zero, so an absent scale means 0 and not the struct default of 1. +TEST(SceneEntitiesCodecTest, ConeDecodesFromAnOmittedTopScale) { + std::vector cylinder; + pb::appendTag(cylinder, 3, 1); + pb::appendDouble(cylinder, 1.0); // bottom_scale; top_scale (field 4) omitted + + std::vector entity; + pb::appendTag(entity, 10, 2); + pb::appendLenDelim(entity, cylinder); + + std::vector bytes; + pb::appendTag(bytes, 2, 2); + pb::appendLenDelim(bytes, entity); + + auto out = deserializeSceneEntities(bytes.data(), bytes.size()); + ASSERT_TRUE(out.has_value()) << out.error(); + ASSERT_EQ(out->entities.size(), 1u); + ASSERT_EQ(out->entities[0].cylinders.size(), 1u); + EXPECT_EQ(out->entities[0].cylinders[0].bottom_scale, 1.0); + EXPECT_EQ(out->entities[0].cylinders[0].top_scale, 0.0); +} + } // namespace } // namespace PJ From ce88a64b480ed6eeeb6ca8cc4ff1fd9fd5fd9c23 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sat, 14 Nov 2026 09:30:32 +0100 Subject: [PATCH 2/2] fix(builtin): annotation scalars and the empty MeshMap follow the zero rule Review findings on the previous commit. An absent annotation thickness, circle diameter or font size decoded as 2, 1 and 14. They are ordinary proto3 scalars, and a thickness of 0 is a real value (a filled shape with no outline) that a standard encoder omits. They now decode as 0; colours still keep their default while the whole colour message is absent. deserializeBuiltinObject(kMeshMap) returned a default-constructed MeshMap (position_type FLOAT32) for the empty payload, while a payload that carries any other field and omits position_type is rejected. The empty payload is now rejected the same way. Tests: both of the above, a cone with only top_scale written, and a malformed PointField followed by a well-formed field. Co-Authored-By: Claude Fable 5.2 --- CHANGELOG.md | 7 +++++- docs/builtin_type.md | 4 ++-- pj_base/src/builtin/builtin_object_codec.cpp | 5 +++- .../src/builtin/image_annotations_codec.cpp | 11 +++++---- pj_base/tests/builtin_object_codec_test.cpp | 8 +++++++ .../tests/image_annotations_codec_test.cpp | 23 +++++++++++++++++++ pj_base/tests/point_cloud_codec_test.cpp | 4 ++++ pj_base/tests/scene_entities_codec_test.cpp | 15 ++++++++++++ 8 files changed, 69 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23f4d46a..586594e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -171,7 +171,12 @@ Builtin decoders: - **Behaviour (wire):** a colour inside an `ImageAnnotations` message decodes like every other `PJ.Color`: a channel absent from a colour that is present is 0. Alpha used to default to 1 there, so a "no fill" colour written as `{r, g, b, a = 0}` by a standard encoder came back opaque. A - colour message that is absent altogether keeps its documented default. + colour message that is absent altogether keeps its documented default. The annotation scalars + follow the same rule: an absent `thickness`, circle `diameter` or `font_size` is 0, where it used + to decode as 2, 1 and 14. A consumer that wants a default for 0 applies it when drawing. +- `deserializeBuiltinObject(kMeshMap, ...)` rejects the empty payload. It returned a + default-constructed `MeshMap` with `position_type` FLOAT32, while any other payload that omits + the field is rejected. - A NaN colour channel decodes as 0; it used to reach an undefined float-to-integer conversion. ## [0.40.0] diff --git a/docs/builtin_type.md b/docs/builtin_type.md index afceee8d..596642bd 100644 --- a/docs/builtin_type.md +++ b/docs/builtin_type.md @@ -133,8 +133,8 @@ encoder and from a later SDK both read correctly: - **An absent field is the proto3 zero value**, even where the C++ struct default differs (`PointCloud::is_dense`, `PointCloud::height`, - `PointField::count`, `CylinderPrimitive::bottom_scale` / `top_scale`, a colour - channel). A standard encoder omits a zero, so a decoder that kept the struct + `PointField::count`, `CylinderPrimitive::bottom_scale` / `top_scale`, an + annotation's `thickness`, `radius` and `font_size`, a colour channel). A standard encoder omits a zero, so a decoder that kept the struct default would turn `is_dense = false` into `true`. The SDK's own serializers write these fields explicitly. A whole submessage that is absent may keep a documented default (an annotation's colour, for example). diff --git a/pj_base/src/builtin/builtin_object_codec.cpp b/pj_base/src/builtin/builtin_object_codec.cpp index 22a333ae..41ab3be8 100644 --- a/pj_base/src/builtin/builtin_object_codec.cpp +++ b/pj_base/src/builtin/builtin_object_codec.cpp @@ -159,7 +159,10 @@ Expected deserializeBuiltinObject(sdk::BuiltinObjectType typ case sdk::BuiltinObjectType::kGridMap: return deserializeAs(data, size, deserializeGridMap); case sdk::BuiltinObjectType::kMeshMap: - return deserializeAs(data, size, deserializeMeshMap); + // Not deserializeAs: the empty message has position_type UNKNOWN, which + // is not a valid mesh, so it is rejected like any other payload that + // omits the field instead of becoming a default-constructed MeshMap. + return eraseDecoded(deserializeMeshMap(data, size)); case sdk::BuiltinObjectType::kMeshMapUpdate: return deserializeAs(data, size, deserializeMeshMapUpdate); case sdk::BuiltinObjectType::kNone: diff --git a/pj_base/src/builtin/image_annotations_codec.cpp b/pj_base/src/builtin/image_annotations_codec.cpp index f3142987..bd503bf7 100644 --- a/pj_base/src/builtin/image_annotations_codec.cpp +++ b/pj_base/src/builtin/image_annotations_codec.cpp @@ -204,10 +204,13 @@ bool decodePointsAnnotation(Reader& reader, PointsAnnotation& out) { } bool decodeCircleAnnotation(Reader& reader, CircleAnnotation& out) { + // Colours keep their default while the whole colour message is absent. The + // scalars start from the proto3 zero value, which a standard encoder omits: + // a thickness of 0 (a filled circle with no outline) must stay 0. out.color = {0, 255, 0, 255}; out.fill_color = {0, 0, 0, 0}; - out.thickness = 2.0; - out.radius = 1.0; + out.thickness = 0.0; + out.radius = 0.0; while (!reader.eof()) { Tag tag; @@ -272,7 +275,7 @@ bool decodeCircleAnnotation(Reader& reader, CircleAnnotation& out) { bool decodeTextAnnotation(Reader& reader, TextAnnotation& out) { out.color = {255, 255, 255, 255}; - out.font_size = 14.0; + out.font_size = 0.0; // the proto3 zero value; the struct default is 14 while (!reader.eof()) { Tag tag; @@ -399,7 +402,7 @@ Expected deserializeImageAnnotations(const uint8_t* data, case 2: { PointsAnnotation points; points.color = {0, 255, 0, 255}; - points.thickness = 2.0; + points.thickness = 0.0; // the proto3 zero value; the struct default is 2 if (!decodePointsAnnotation(nested, points)) { return unexpected(std::string("ImageAnnotations wire: PointsAnnotation decode failed")); } diff --git a/pj_base/tests/builtin_object_codec_test.cpp b/pj_base/tests/builtin_object_codec_test.cpp index 468c1394..44981412 100644 --- a/pj_base/tests/builtin_object_codec_test.cpp +++ b/pj_base/tests/builtin_object_codec_test.cpp @@ -155,4 +155,12 @@ TEST(BuiltinObjectCodec, EmptyPointCloudPayloadHoldsTheProto3ZeroValues) { EXPECT_FALSE(cloud->is_dense); } +// The empty MeshMap message has position_type UNKNOWN, which no mesh may have. +// It is rejected, like a payload that carries other fields and omits that one. +TEST(BuiltinObjectCodec, EmptyMeshMapPayloadIsRejectedLikeAnyOtherWithoutAPositionType) { + EXPECT_FALSE(PJ::deserializeBuiltinObject(PJ::sdk::BuiltinObjectType::kMeshMap, nullptr, 0)); + const uint8_t frame_id_only[] = {0x12, 0x00}; + EXPECT_FALSE(PJ::deserializeBuiltinObject(PJ::sdk::BuiltinObjectType::kMeshMap, frame_id_only, 2)); +} + } // namespace diff --git a/pj_base/tests/image_annotations_codec_test.cpp b/pj_base/tests/image_annotations_codec_test.cpp index 9057767b..aebc15ed 100644 --- a/pj_base/tests/image_annotations_codec_test.cpp +++ b/pj_base/tests/image_annotations_codec_test.cpp @@ -459,5 +459,28 @@ TEST(ImageAnnotationCodecTest, NaNColorChannelDecodesAsZero) { EXPECT_EQ(out->circles[0].color, (sdk::ColorRGBA{0, 0, 0, 255})); } +// The scalar fields of an annotation follow the proto3 rule too: absent is 0, +// whatever the struct default. An outline thickness of 0 is a real value (a +// filled shape with no outline), and a standard encoder omits it. +TEST(ImageAnnotationCodecTest, OmittedAnnotationScalarsDecodeAsZero) { + const std::vector bytes = { + 0x0A, 0x00, // one empty CircleAnnotation + 0x12, 0x00, // one empty PointsAnnotation + 0x1A, 0x00, // one empty TextAnnotation + }; + auto out = deserializeImageAnnotations(bytes.data(), bytes.size()); + ASSERT_TRUE(out.has_value()) << out.error(); + ASSERT_EQ(out->circles.size(), 1u); + ASSERT_EQ(out->points.size(), 1u); + ASSERT_EQ(out->texts.size(), 1u); + EXPECT_EQ(out->circles[0].radius, 0.0); + EXPECT_EQ(out->circles[0].thickness, 0.0); + EXPECT_EQ(out->points[0].thickness, 0.0); + EXPECT_EQ(out->texts[0].font_size, 0.0); + // The colours were absent as whole messages, so they keep their defaults. + EXPECT_EQ(out->circles[0].color, (sdk::ColorRGBA{0, 255, 0, 255})); + EXPECT_EQ(out->texts[0].color, (sdk::ColorRGBA{255, 255, 255, 255})); +} + } // namespace } // namespace PJ diff --git a/pj_base/tests/point_cloud_codec_test.cpp b/pj_base/tests/point_cloud_codec_test.cpp index adff1fac..7f8bb1ff 100644 --- a/pj_base/tests/point_cloud_codec_test.cpp +++ b/pj_base/tests/point_cloud_codec_test.cpp @@ -134,6 +134,10 @@ TEST(PointCloudCodecTest, MalformedPointFieldFailsTheDecode) { pb::appendLenDelim(bytes, {0x10, 0x80}); // offset: a varint cut short bytes.insert(bytes.end(), {0x02, 0x10, 0x07}); EXPECT_FALSE(deserializePointCloud(bytes.data(), bytes.size()).has_value()); + + // The same bad PointField followed by a well-formed field is rejected too. + const auto valid_suffix = cloudWithField({0x10, 0x80}); + EXPECT_FALSE(deserializePointCloud(valid_suffix.data(), valid_suffix.size()).has_value()); } // A standard proto3 encoder omits every field equal to its zero value, so a diff --git a/pj_base/tests/scene_entities_codec_test.cpp b/pj_base/tests/scene_entities_codec_test.cpp index 10482708..caea5ce5 100644 --- a/pj_base/tests/scene_entities_codec_test.cpp +++ b/pj_base/tests/scene_entities_codec_test.cpp @@ -365,6 +365,21 @@ TEST(SceneEntitiesCodecTest, ConeDecodesFromAnOmittedTopScale) { ASSERT_EQ(out->entities[0].cylinders.size(), 1u); EXPECT_EQ(out->entities[0].cylinders[0].bottom_scale, 1.0); EXPECT_EQ(out->entities[0].cylinders[0].top_scale, 0.0); + + // And the other way up: only top_scale is written. + cylinder.clear(); + pb::appendTag(cylinder, 4, 1); + pb::appendDouble(cylinder, 1.0); + entity.clear(); + pb::appendTag(entity, 10, 2); + pb::appendLenDelim(entity, cylinder); + bytes.clear(); + pb::appendTag(bytes, 2, 2); + pb::appendLenDelim(bytes, entity); + out = deserializeSceneEntities(bytes.data(), bytes.size()); + ASSERT_TRUE(out.has_value()) << out.error(); + EXPECT_EQ(out->entities[0].cylinders[0].bottom_scale, 0.0); + EXPECT_EQ(out->entities[0].cylinders[0].top_scale, 1.0); } } // namespace