Add Flatbuffer Schema Support #70

snosenzo · 2022-11-08T20:59:49Z

Add code to generate flatbuffer schema files: .fbs.
Added test for expected output

To test compilation: install cmake and flatbuffer (via homebrew or other means) then run this command from the repo root directory: flatc --ts -o ./schemas/flatbuffer/output ./schemas/flatbuffer/foxglove/**.fbs
and see that it generates the schema files in the schemas/flatbuffer/output directory.

Few Open Questions about this implementation:

~~should I use the protobuf enums names?~~
- Should enums by namespaced by their parent ?
~~just to be sure there aren’t any wellknown types for time and duration like protobuf/ros?~~
- ~~currently using something similar to the way we handle typescript generation~~
- none exist
~~Do we want defaults for primitive types?~~
- added defaults directly to the schema that are reflected in protobuf
~~should we copy protobuf file structure and namespaces (foxglove.etc)~~?
- all necessary schemas are under the foxglove namespace

- tested compilation is successful

jameskuszmaul-brt · 2022-11-08T22:46:12Z

should I use the protobuf enums names?

just to be sure there aren’t any wellknown types for time and duration like protobuf/ros?

There are no well-known types.

currently using something similar to the way we handle typescript generation

Do we want defaults for primitive types?

Note that the "default defaults" are zero, which is typically reasonable. I'd suggest explicit defaults for enums (because sometimes there is no 0 value for the enum; it's not required, but it helps make things clearer).

incorporated into schema or defaults per primitive added at generation

should we copy protobuf file structure and namespaces (foxglove.etc)?

File structure is generally whatever. Namespaces should definitely be consistent.

should enums be separate files or with their message schema like protobuf?

It doesn't strictly matter if they are in separate files or not. My typical philosophy is to not put them into separate files unless they are logically separate or there is a particular need to (e.g., enums are shared across many different types). Since it looks like you are doing this based on codegen, I think your current approach is fine.

NOTE: the compiler does warn that some CameraCalibration fields are not lowercase snake_case compatible.
/Users/samnosenzo/dev/schemas/schemas/flatbuffer/foxglove/CameraCalibration.fbs:26: 3: warning: field names should be lowercase snake_case, got: D

I think that in order to maintain consistency with the schemas in your other languages, you want to leave it as-is. The fbs compiler will sometimes mess with the names for certain codegen, which is why it watches for this (e.g., for Java, a snake_case name in the fbs file gets turned into a (not)SnakeCase name in the generated java code).

jhurliman · 2022-11-08T23:02:42Z

I think that in order to maintain consistency with the schemas in your other languages

ROS 2 does not support naming a field D, so it would be consistent with at least our ROS 2 generator to rename this to d.

jameskuszmaul-brt · 2022-11-08T23:04:32Z

I think that in order to maintain consistency with the schemas in your other languages

ROS 2 does not support naming a field D, so it would be consistent with at least our ROS 2 generator to rename this to d.

Are the panels that consume these fields on the Studio side case-insensitive? If so, then yeah just make everything lowercase.

jhurliman · 2022-11-08T23:12:50Z

Note that the "default defaults" are zero, which is typically reasonable

Some notable exceptions:

w component of quaternions (1 is better)
r,g,b,a fields of color (1 is better)
x,y,z fields of scale (1 is better)

jameskuszmaul-brt · 2022-11-08T23:28:49Z

Also re: defaults: note that, like protobufs, flatbuffers do have a concept of a field not being populated. It is also the case that the serialization for flatbuffers by default is configured to not serialize fields if the field is set to its default (so has_X() may return false even if the serializer did add_X(default)), although that is configurable https://google.github.io/flatbuffers/classflatbuffers_1_1_flat_buffer_builder.html#a16a8fd46b34ad7727406c37b65b6b27a .

jtbandes · 2022-11-08T23:57:11Z

Is there some compile step we can run in CI to validate these generated files? For example, we do this for protobuf:

schemas/.github/workflows/ci.yml

Lines 39 to 40 in b36014a

    
                 - name: Validate protobuf definitions 
        
                   run: protoc --proto_path=schemas/proto schemas/proto/**/*.proto --descriptor_set_out=/dev/null

jtbandes · 2022-11-09T00:16:30Z

should I use the protobuf enums names?

Protobuf has special names because the types are nested inside message types:

name: "SceneEntityDeletionType",
protobufParentMessageName: "SceneEntityDeletion",
protobufEnumName: "Type",

The enum which is called SceneEntityDeletionType in other languages becomes SceneEntityDeletion.Type in protobuf.

This was done partly because the way protobuf c++ code is generated, the enum names pollute their parent namespace, rather than being namespaced under the enum type itself. So multiple top-level enums with UNKNOWN values would conflict with each other.

I don't know if flatbuffers would have the same problem or not?

jameskuszmaul-brt · 2022-11-09T00:19:36Z

The enum which is called SceneEntityDeletionType in other languages becomes SceneEntityDeletion.Type in protobuf.

This was done partly because the way protobuf c++ code is generated, the enum names pollute their parent namespace, rather than being namespaced under the enum type itself. So multiple top-level enums with UNKNOWN values would conflict with each other.

I don't know if flatbuffers would have the same problem or not?

It depends on where/how the types are used.

In C++, flatc provides a --scoped-enums flag to let you choose how you want your enums generated. We always turn that on in our projects, but I think in upstream flatbuffers it is off by default. Other languages have language-specific behavior.

- final tweaks to generate script to support nested bytes - update time and duration to be more consistent with protobuf types

snosenzo · 2022-11-09T16:38:11Z

r,g,b,a fields of color (1 is better) & x,y,z fields of scale (1 is better)

Is it fine to just add = 1.0 to all fields named this way that are primitives? Or should I hard-code certain ones that should apply to these?

Enum Scoping and Namespaces

So as it stands right now all of the enums except for the pre-determined (ByteVectorForNesting, Time, Duration) ones all live in the foxglove namespace.

should I put these into the foxglove namespace?
It seems like it might also be desireable to have trailing namespaces for enums that belong to protobuf parents. I can add specific namespace definitions to their schema to not pollute the foxglove namespace. so the LineType enum would be namespace foxglove.LinePrimitive. Is this more desirable than having them all in the foxglove namespace?

flatc provides a --scoped-enums

It's a little unclear to me what this uses to determine scoping. Is it just if it's in the file with a root_type?

- lowercase field names for compliance with flatc warnings

snosenzo · 2022-11-09T16:52:30Z

Is there some compile step we can run in CI to validate these generated files? For example, we do this for protobuf:

looked into this and can't find a github action for it like there is for protobuf unfortunately. Is there somewhere else I should look or do you know how easy it would be to write one? I just don't know that much about making github actions

jameskuszmaul-brt · 2022-11-09T17:25:41Z

It's a little unclear to me what this uses to determine scoping. Is it just if it's in the file with a root_type?

In C++ it generates a Scoped Enum

I.e., with --scoped-enums if you have a fbs file like

namespace foo;

enum Abc : byte {
  A = 0,
  B = 1
}

Then the generated C++ code will be

namespace foo {
enum class Abc : int8_t {
  A = 0,
  B = 1,
  MIN = A,
  MAX = B
};
}

Without --scoped-enums, it looks like flatc's default C++ code generates

namespace foo {
enum Abc : int8_t {
  Abc_A = 0,
  Abc_B = 1,
  Abc_MIN = A,
  Abc_MAX = B
};
}

They also seem to have a flag to strip those prefixes off, but I doubt most people use it.

Other languages I think tend to have some sort of scoping to their enums by default, so this is less of an issue outside of C++.

snosenzo · 2022-11-09T17:33:25Z

ah I see, thanks for clarifying! Wasn't sure if it was related to the namespacing of enums

jtbandes

How does someone go about merging flatbuffer schemas and their includes into one blob, in order to put them in an mcap/websocket schema? Is it acceptable that each file has root_type in it? I would guess not, and we might want to remove root_type from our generated fbs files, but maybe there is some convenient way of merging them (like protoc's --descriptor_set_out)?

jtbandes · 2022-11-10T00:48:04Z

scripts/updateGeneratedFiles.ts

+      path.join(outDir, "flatbuffer/foxglove", "ByteVectorForNesting.fbs"),
+      BYTE_VECTOR_FB,
+    );
+    await fs.writeFile(path.join(outDir, "flatbuffer/foxglove", "Time.fbs"), TIME_FB);


Is it normal to use the foxglove package dir for flatbuffers?

Should this be named flatbuffer or flatbuffers? Given that the project website starts with "FlatBuffers is an efficient cross platform serialization library..." I'm guessing probably flatbuffers is the more accepted name?

The github repo is https://github.com/google/flatbuffers and the C++ includes are in a flatbuffers folder.

There's no real prescribed folder structure for flatbuffers as far as I can tell. I thought I'd follow the protobuf pattern of making it follow the namespace, but that's probably not necessary. Removed. Good callout on flatbuffer->flatbuffers though