proto3 packed repeated fields

[keunhong]

protobuf.js currently does not handle proto3 repeated fields properly.

In proto3, all repeated fields are packed by default but failing to designate a repeated field as [packed=true] causes protobuf.js to incorrectly raise an error:

protobuf.js:3679 Uncaught Error: Illegal wire type for field Message.Field .Holoscanner.Proto.Mesh.triangles: 2 (0 expected)

Adding [packed=true] fixes this issue but this is not valid syntax is proto3.

Specifically:

message Mesh {
    uint32 mesh_id = 2;
    uint64 timestamp = 3;

    // We need the camera position in case we want to do space carving.
    Vec3D cam_position = 100;
    Vec4D cam_rotation = 101;

    repeated Vec3D vertices = 200;
    repeated int32 triangles = 201;
}

Causes protobuf.js to raise the iIlegal wire type error while it works fine in other protobuf clients.

message Mesh {
    uint32 mesh_id = 2;
    uint64 timestamp = 3;

    // We need the camera position in case we want to do space carving.
    Vec3D cam_position = 100;
    Vec4D cam_rotation = 101;

    repeated Vec3D vertices = 200;
    repeated int32 triangles = 201 [packed=true];
}

This works since protobuf.js correctly treats triangles as a packed field.

[willliu]

Dear decodeIO:

Thank you for your attention. In short, proto3 in C++ seems to treat "repeated double =1" as "[packed = true]" for default, and JS seems to treat as "[packed = false]".

Here is a comment for a solution: always put "[packed=true/false]" explicitly.

Besides my comment, I really appreciate your amazing/official standard work!

Thanks!

[keunhong]

Explicitly putting [packed=true/false] is not valid syntax in proto3.

[willliu]

This is interesting...

[tvald]

from https://developers.google.com/protocol-buffers/docs/encoding#packed

In proto3, repeated fields are packed by default. Protocol buffer parsers must be able to parse repeated fields that were compiled as packed as if they were not packed, and vice versa.

[rastapasta]

Could anyone point me to the place in the code where the parsing of "repeated" field type happens? Would love to fork it with just a quick-and-dirty patch for always setting repeated fields as packed. Thanks in advance, would help a lot!

[laverdet]

+1 this is a huge issue for proto3 compatibility

@rastapasta I patched this myself in BuilderPrototype.resolveAll -- add this add the end of the function:

if (this.ptr.repeated && (this.ptr.type.wireType === 0 || this.ptr.type.wireType === 1 || this.ptr.type.wireType === 5)) this.ptr.options.packed = true;

If nothing happens on this issue I'll submit a PR with a slightly cleaner patch.

[laverdet]

PR submitted. I pushed a fixed version of the library to npm as protobufjs-laverdet-proto3-packed-fix for users who want the fix immediately.

[englercj]

@laverdet Usually it is easier to use a git url for a temporary package since npm packages are immutable and forever :(

[dcodeIO]

Closing this for now.

protobuf.js 6.0.0

Feel free to send a pull request if this is still a requirement.

[arhuaco]

I am having this issue with protobufjs 5.0.1 as well (cannot move to 6.x.x ATM). If we sent a PR like this one, would you consider adding it?

https://github.com/cyraxx/node-pogo-protos/commit/17b597513cb8c7aab777cd1ea5fe7228cd15c166

[arhuaco]

Never mind. I'll add [packed=true] as it doesn't break compilation of the C++ protobuf3 file. It doesn't look pretty there, but we only have this in one place. BTW, thanks for this package.