Add optional shape enums in sdk

[thswansonfxg]

Changelog

Add point_style optional enum field to LocationFix schema, allowing users to customize the marker style (dot, diamond, square, plus, cross, pin) when visualizing locations on a map.

Docs

The generated schemas/README.md includes the new PointStyle enum and point_style field documentation. No additional docs PR is needed.

Description

This PR adds a new PointStyle enum and optional point_style field to the LocationFix schema across all SDK bindings (Rust,
Python, C++, C, TypeScript, ROS, Protobuf, FlatBuffer, JSON Schema, OMG IDL).

Changes:

• New PointStyle enum with values: DOT (default, 0), DIAMOND (1), SQUARE (2), PLUS (3), CROSS (4), PIN (5)
• point_style field on LocationFix — optional in all bindings (Option in Rust, std::optional in C++, *const pointer in C, Option in Python)
• Generator fix (generatePyclass.ts) — Python SDK codegen was not handling optional enum fields, generating field as i32 instead of field.map(|s| s as i32)
• Generator fix (generateSdkCpp.ts) — Support multiple enums per parent schema, handle optional enum to conversion with nullptr for absent values
• Generator fix (generateSdkRustCTypes.ts) — Optional enum fields in C FFI now use pointer types for proper nullability
• Serde support (schemas.rs) — Added serde_enum_mod_optional! macro for serializing/deserializing Option enum fields as string names in JSON and raw i32 in binary formats, with round-trip tests
• Proto gen fix (foxglove_proto_gen) — Detects proto3_optional enum fields and emits the optional serde macro variant

https://linear.app/foxglove/issue/ERT-1612/add-optional-attribute-to-control-point-shape-in-locationfix-messages

[linear]

ERT-1612 add optional attribute to control point shape in locationfix messages

[claude]

Claude finished @thswansonfxg's task in 2m 23s —— View job

---

Summary

This PR adds a new PointStyle enum and optional point_style field to the LocationFix schema across all SDK bindings (Rust, Python, C++, C, TypeScript, ROS, Protobuf, FlatBuffer, JSON Schema, OMG IDL). The implementation includes generator fixes for handling optional enum fields correctly in Python, C++, and C FFI, plus a new serde macro for optional enum serialization in Rust.

The PR addresses the issues raised by Cursor Bugbot in earlier reviews:

• ✅ C FFI optional enum: Now uses *const FoxglovePointStyle pointer type for nullability
• ✅ C++ reinterpret_cast UB: Now uses arena allocation + static_cast instead of reinterpret_cast
• ✅ Missing serde(default): Added to handle missing keys during JSON deserialization

Blockers

None

Suggestions

1. PR description should use "Fixes:" format for Linear link

The Linear issue link at the bottom of the PR description should use the Fixes: prefix format per the PR template:

Fixes: [ERT-1612](https://linear.app/foxglove/issue/ERT-1612/add-optional-attribute-to-control-point-shape-in-locationfix-messages)

2. Docs section is empty

The PR template asks for documentation updates or a tracking ticket. Since this adds a user-facing schema field, consider:

• Linking to a docs PR or ticket, or
• Noting "None" explicitly if the schema README update is sufficient

3. Consider adding a test for the C FFI optional enum round-trip

The Rust serde tests (test_location_fix_json_roundtrip_with_point_style, etc.) are good, but there's no explicit test verifying the C FFI *const FoxglovePointStyle → Option<i32> conversion handles both nullptr (None) and valid pointer (Some) cases correctly. The conversion at c/src/generated_types.rs:3303 looks correct:

point_style: unsafe { self.point_style.as_ref() }.map(|&v| v as i32),

But an integration test would catch regressions if the generator changes.

4. Test fixture update is good, but snapshot diffs are noisy

The test fixtures now include field_optional_enum which exercises the new code paths. The snapshot diffs show whitespace/indentation changes unrelated to the feature - this appears to be a formatting normalization. Not a blocker, just noting it makes the diff harder to review.

5. Minor: color field uses reinterpret_cast in C++ but point_style uses arena allocation

Looking at cpp/foxglove/src/schemas.cpp:1760:

dest.color = src.color ? reinterpret_cast<const foxglove_color*>(&*src.color) : nullptr;

The color field (a nested Color struct) uses reinterpret_cast while point_style (an enum) correctly uses arena allocation. This is fine because Color is in the copyTypes set (same memory layout in C++ and C), but the inconsistency might confuse future readers. A comment explaining why enums need arena allocation while some structs don't would help.

---

Overall this is solid work - the optional enum handling across all the SDK bindings is thorough, and the fixes for the UB and serde issues are correct. 👍

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[jtbandes]

This feels like a good direction, I'm just not sure if I have quite enough expertise on the Rust and C++ (particularly Arena) side of things to give a definitive stamp 😬

[jtbandes]

@eloff @ericmlujan could you folks review this part? I'm not familiar with this code at all and @gasmith is out -- I do wonder if there's a way we could combine this with the non-optional macro rather than having two whole macros for it? but not sure how feasible that suggestion is

[thswansonfxg]

I'm certainly not confident enough in Rust macros to combine them nicely, especially for SDK level code. This was done because it was easier for me to understand and easier for me to read.

Super happy to change this if someone better at rust has some ideas.

[jtbandes]

is this actually unsafe? I think this is for someone who knows more Rust than me to answer

[thswansonfxg]

I think yes because self.point_style is a *const FoxglovePointStyle — a raw pointer in the C FFI struct.

But we also talked about this in person. Someone who knows rust better might need to stamp this.

[jtbandes]

I am wondering if this whole thing could possibly be replaced with dest.${dstName} = reinterpret_cast<const ${cEnumType}*>(src.${srcName})?

I'm not sure if that would actually work or be safe - it just came to mind since the generated code looked a bit complicated and I think these types are compatible.

If that doesn't work, it might be nice to actually put some line breaks in this string so it's more readable here (though it does get auto-formatted in the generated files)

[thswansonfxg]

That was implemented because of this cursor comment here: #870 (comment)

Further reading says that maybe its totally safe? I am happy to change this, but its being flagged because of garbage bytes and the potential of undefined behavior

[jtbandes]

ah ok, I missed that the sizes are different, I guess that makes sense

[jtbandes]

I wonder if we should do something like

Suggested change

	name: "PointStyle",
	description: "Style of point used for map visualization",
	parentSchemaName: "LocationFix",
	protobufEnumName: "PointStyle",
	name: "MapPointStyle", //(or LocationFixPointStyle?)
	description: "Style of point used for map visualization",
	parentSchemaName: "LocationFix",
	protobufEnumName: "PointStyle",

?

Although it's also making me realize that what is currently called "protobufEnumName" should maybe be used for any language where we nest enums inside the schemas (which it seems includes c++ too)...

[thswansonfxg]

I think we want to have PointStyle as eventually the goal might be to have these point styles shared across a few different panels. I agree that maybe we need to update protobufEnumName to just be enumName. Also happy to make those changes