fix(builtin): decoders skip unknown fields and read absent fields as zero (1.0.0)

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,32 @@ 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. 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]
 
 Host contract: extended: pj.mcp_endpoint.v1, PJ_mcp_endpoint_host_vtable_t::acquire_endpoint,

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`, 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).
+- **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

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.

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;
 }

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;
 }
 

pj_base/src/builtin/builtin_object_codec.cpp

@@ -117,6 +117,14 @@ Expected<sdk::BuiltinObject> deserializeBuiltinObject(sdk::BuiltinObjectType typ
     case sdk::BuiltinObjectType::kImage:
       return deserializeAs<sdk::Image>(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<sdk::PointCloud>(data, size, deserializePointCloud);
     case sdk::BuiltinObjectType::kDepthImage:
       return deserializeAs<sdk::DepthImage>(data, size, deserializeDepthImage);
@@ -151,7 +159,10 @@ Expected<sdk::BuiltinObject> deserializeBuiltinObject(sdk::BuiltinObjectType typ
     case sdk::BuiltinObjectType::kGridMap:
       return deserializeAs<sdk::GridMap>(data, size, deserializeGridMap);
     case sdk::BuiltinObjectType::kMeshMap:
-      return deserializeAs<sdk::MeshMap>(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<sdk::MeshMapUpdate>(data, size, deserializeMeshMapUpdate);
     case sdk::BuiltinObjectType::kNone:

pj_base/src/builtin/geometry_codec.hpp

@@ -16,6 +16,7 @@
 // Inline-only; not exposed through the public include path.
 
 #include <algorithm>
+#include <cmath>
 #include <cstdint>
 
 #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<uint8_t>(value * 255.0 + 0.5);
 }
 

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) {
@@ -247,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;
@@ -315,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;
@@ -442,7 +402,7 @@ Expected<sdk::ImageAnnotations> 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"));
         }

pj_base/src/builtin/log_codec.cpp

@@ -48,7 +48,7 @@ std::vector<uint8_t> 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<sdk::Log> 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;

pj_base/src/builtin/plot_markers_codec.cpp

@@ -4,6 +4,7 @@
 #include "pj_base/builtin/plot_markers_codec.hpp"
 
 #include <algorithm>
+#include <cmath>
 #include <cstdint>
 #include <optional>
 #include <string>
@@ -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<uint8_t>(value * 255.0 + 0.5);
 }
 

pj_base/src/builtin/point_cloud_codec.cpp

@@ -77,6 +77,9 @@ Expected<sdk::PointCloud> 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) {

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)) {

pj_base/src/builtin/protobuf_wire.hpp

@@ -322,18 +322,27 @@ 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 <typename FieldHandler>
 [[nodiscard]] inline bool parseFields(Reader& reader, FieldHandler&& handler) {
   while (!reader.eof()) {
     Tag tag;
     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;
     }
   }

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:

pj_base/tests/builtin_object_codec_test.cpp

@@ -143,4 +143,24 @@ 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<PJ::sdk::PointCloud>();
+  ASSERT_NE(cloud, nullptr);
+  EXPECT_EQ(cloud->height, 0u);
+  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