Add Godot MessagePack integration - #1025
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@Starwer I'd welcome your feedback on this as a godot user. |
There was a problem hiding this comment.
Pull request overview
Adds a first-party Nerdbank.MessagePack.Godot integration package that registers MessagePack converters for common Godot Engine value types, enabling migration from MessagePack-CSharp/MessagePackGodot without adopting its resolver model.
Changes:
- Introduces
Nerdbank.MessagePack.GodotwithWithGodotConverters()and positional-array converters for 16 Godot value types. - Adds a new test project with round-trip coverage and a couple of wire-compatibility behaviors.
- Updates repo documentation and packaging metadata (DocFX docs, root README badge, package references).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Nerdbank.MessagePack.Godot.Tests/Nerdbank.MessagePack.Godot.Tests.csproj | New test project for the Godot integration. |
| test/Nerdbank.MessagePack.Godot.Tests/GodotConverterTests.cs | Adds round-trip and selected wire-format behavior tests for Godot converters. |
| test/dirs.proj | Includes the new Godot test project in the test build graph. |
| src/Nerdbank.MessagePack.Godot/README.md | Package-level README with basic usage and doc link. |
| src/Nerdbank.MessagePack.Godot/Nerdbank.MessagePack.Godot.csproj | New shipping package project with multi-targeting and NuGet metadata. |
| src/Nerdbank.MessagePack.Godot/GodotMessagePackSerializerExtensions.cs | Adds WithGodotConverters() serializer configuration entrypoint. |
| src/Nerdbank.MessagePack.Godot/GodotConverters.cs | Implements converters for supported Godot value types using positional arrays. |
| README.md | Adds NuGet badge for the new Godot package. |
| docfx/docs/toc.yml | Adds Godot documentation page to DocFX TOC. |
| docfx/docs/godot.md | New documentation for installation/configuration/supported types/migration notes. |
| docfx/docfx.json | Includes the new Godot project in DocFX API generation inputs. |
| Directory.Packages.props | Adds centralized version for the GodotSharp dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Nerdbank.MessagePack.Godot/GodotConverters.cs:587
stackalloc float[16]creates an uninitialized span, so when the incoming array has fewer than 16 elements the remaining entries may contain garbage values. That would makeProjectiondeserialization nondeterministic for truncated/forward-compatible payloads. Clear the span (or otherwise initialize missing elements) before populating it.
Span<float> values = stackalloc float[16];
for (int i = 0; i < Math.Min(length, values.Length); i++)
{
values[i] = reader.ReadSingle();
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Nerdbank.MessagePack.Godot/GodotConverters.cs:23
- SkipRemaining silently accepts arrays that are shorter than the expected length, which will deserialize corrupted/truncated payloads into default(0) components (e.g., Vector2 missing Y becomes 0) without any error. Since the forward-compat story is about additional elements, it seems safer to throw when actualLength < expectedLength and only skip when actualLength > expectedLength.
protected static void SkipRemaining(ref MessagePackReader reader, SerializationContext context, int actualLength, int expectedLength)
{
for (int i = expectedLength; i < actualLength; i++)
{
reader.Skip(context);
}
}
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
src/Nerdbank.MessagePack.Godot/GodotConverters.cs:9
- File-level suppression of SA1107/SA1503 looks avoidable here: the only multi-statement lines and omitted-brace cases are local and can be rewritten to match the style used throughout the rest of the repo (and avoid blanket suppressions that can hide future issues). After reformatting the few affected spots, consider dropping these suppressions.
#pragma warning disable SA1107 // Code should not contain multiple statements on one line
#pragma warning disable SA1137 // Elements should have the same indentation
#pragma warning disable SA1402 // File may only contain a single type
#pragma warning disable SA1503 // Braces should not be omitted
#pragma warning disable SA1600 // Elements should be documented
#pragma warning disable SA1649 // File name should match first type name
src/Nerdbank.MessagePack.Godot/GodotConverters.cs:113
Vector2IConverter.Readuses a one-lineif/elseinside the loop, which is inconsistent with the rest of the converters and (together withVector2Converter.Read) forces a file-level SA1503 suppression. Aswitchkeeps the same pattern as other converters in this file.
public override Vector2I Read(ref MessagePackReader reader, SerializationContext context)
{
context.DepthStep();
int length = reader.ReadArrayHeader();
src/Nerdbank.MessagePack.Godot/GodotMessagePackSerializerExtensions.cs:23
WithGodotConverters()appendsGodotConverterFactory.Instanceevery time it’s called, so repeated calls will growConverterFactorieswith duplicates (extra work during converter resolution and surprising behavior compared to otherWith*helpers). Consider making this method idempotent by returning the same serializer when the factory is already present.
public static MessagePackSerializer WithGodotConverters(this MessagePackSerializer serializer)
{
ArgumentNullException.ThrowIfNull(serializer);
return serializer with { ConverterFactories = [.. serializer.ConverterFactories, GodotConverterFactory.Instance] };
}
src/Nerdbank.MessagePack.Godot/GodotConverters.cs:86
Vector2Converter.Readuses a one-lineif/elseinside the loop, which is inconsistent with theswitch-based style used by the other converters in this file and is the main reason SA1503 is being disabled. A smallswitchkeeps the pattern consistent and avoids omitted-brace constructs.
This issue also appears on line 110 of the same file.
public override Vector2 Read(ref MessagePackReader reader, SerializationContext context)
{
context.DepthStep();
int length = reader.ReadArrayHeader();
src/Nerdbank.MessagePack.Godot/GodotConverters.cs:581
ProjectionConverter.Writecurrently packs fourwriter.Write(...)calls onto each line. This is harder to scan/blame and is the reason SA1107 is suppressed for the entire file. Splitting to one statement per line keeps StyleCop enabled and matches the surrounding converter style.
internal static readonly ProjectionConverter Instance = new();
public override void Write(ref MessagePackWriter writer, in Projection value, SerializationContext context)
{
context.DepthStep();
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Nerdbank.MessagePack.Godot/GodotMessagePackSerializerExtensions.cs:23
WithGodotConverters()currently always appendsGodotConverterFactory.Instanceand usesArgumentNullException.ThrowIfNull, which is inconsistent with the rest of the serializer extension APIs (they typically useMicrosoft.Requires) and makes the method non-idempotent (calling it twice adds duplicate factories and adds extra work to each converter lookup). Consider switching toRequires.NotNulland returning early when the factory is already present.
public static MessagePackSerializer WithGodotConverters(this MessagePackSerializer serializer)
{
ArgumentNullException.ThrowIfNull(serializer);
if (serializer.ConverterFactories.Contains(GodotConverterFactory.Instance))
{
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Nerdbank.MessagePack.Godot/GodotMessagePackSerializerExtensions.cs:18
- The XML doc comment for
WithGodotConverterssays it "creates a copy" of the serializer, but the method is intentionally idempotent and returns the original instance unchanged when the converters are already present. Consider updating the summary/returns text to reflect this behavior so the docs match the implementation (and theWithGodotConverters_IsIdempotenttest).
/// <summary>
/// Creates a copy of a serializer configured to serialize Godot Engine value types.
/// </summary>
/// <param name="serializer">The serializer to configure.</param>
/// <returns>A serializer configured with converters for Godot Engine value types.</returns>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
test/Nerdbank.MessagePack.Godot.Tests/GodotConverterTests.cs:99
ValueTypes_RoundTripusesReflectionTypeShapeProvider(reflection) for all 16 types viaRoundTrip<T>, which doesn’t exercise the package’s claimed NativeAOT-friendly path (source-generated shapes) except in the few explicit...<T, GodotShapes>tests. UsingGodotShapesfor the round-trip helper would validate that the converters work end-to-end with generated shapes across all supported types (and avoids depending on reflection in this new test project).
private static T RoundTrip<T>(T value)
{
ITypeShape<T> shape = ReflectionTypeShapeProvider.Default.GetTypeShapeOrThrow<T>();
return Serializer.Deserialize(Serializer.Serialize(value, shape, TestContext.Current.CancellationToken), shape, TestContext.Current.CancellationToken)!;
}
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
I'm not too sure what I should expect here from this change. I could serialize |
|
@Starwer I'm delighted to hear that the Godot types worked out of the box for you. Nerdbank.MessagePack can serialize many types out of the box that MessagePack-CSharp required custom formatters or attributes for. Possible goodness from this PR could be faster conversion performance on account of specialized converters, and as you say, schema compatibility with messagepack-csharp. But TBH I haven't tested the baseline to see the impact this package has over baseline on either of those two metrics. |
Baseline vs
|
| Baseline | Godot converters | |
|---|---|---|
| Layout | Named maps of public members | Compact positional arrays (MessagePackGodot-compatible) |
Vector2(1.5,-2.5) |
{"X":1.5,"Y":-2.5} (15 B) |
[1.5,-2.5] (11 B) |
Transform3D sample |
Nested maps (280 B) | 12-float array (61 B) |
Baseline also serializes derived/redundant members that do not need to be on the wire (Rect2.End, Aabb.End, Transform2D.Rotation/Scale/Skew, both row and column views on Basis, Plane.Normal plus X/Y/Z, OkHSL and other computed Color props, etc.). Neither side can deserialize the other’s bytes.
Payload size (baseline / godot)
| Value | Baseline | Godot | Ratio |
|---|---|---|---|
Vector2 |
15 B | 11 B | 1.4× |
Rect2 |
64 B | 21 B | 3.0× |
Transform3D |
280 B | 61 B | 4.6× |
Transform3D[100] |
28003 B | 6103 B | 4.6× |
Performance (BenchmarkDotNet, .NET 8 Release)
Godot converters are consistently faster:
| Operation | Baseline | Godot | Speedup |
|---|---|---|---|
Vector2 serialize |
65.8 ns | 34.0 ns | 1.9× |
Vector2 deserialize |
132.8 ns | 67.4 ns | 2.0× |
Rect2 serialize |
168.6 ns | 38.8 ns | 4.3× |
Rect2 deserialize |
411.2 ns | 97.0 ns | 4.2× |
Aabb serialize |
208.5 ns | 43.1 ns | 4.8× |
Aabb deserialize |
503.4 ns | 80.0 ns | 6.3× |
Transform3D serialize |
708.3 ns | 67.0 ns | 10.6× |
Transform3D deserialize |
1763.7 ns | 108.1 ns | 16.3× |
Vector2[100] serialize |
3.57 µs | 0.65 µs | 5.5× |
Vector2[100] deserialize |
8.81 µs | 2.16 µs | 4.1× |
Takeaway
Even though many Godot structs serialize “for free,” Nerdbank.MessagePack.Godot still earns its keep:
- MessagePack-CSharp / MessagePackGodot wire compatibility (migration)
- Much smaller payloads (no property names; no derived members)
- Materially faster convert paths (~2× on simple vectors, ~4–16× on composite transforms)
- Safer member selection — especially
Color, where baseline would serialize native/computed properties that are unnecessary and can crash outside the Godot engine
Godot users migrating from MessagePack-CSharp need first-party converters for core engine value types without adopting its resolver model.
Adds
Nerdbank.MessagePack.Godot, configured withWithGodotConverters(), with compact positional-array converters for 16 Godot value types. The wire layouts matchMessagePackGodot, allowing existing payloads to be read during migration and retaining forward-compatible reads of appended array elements.Also adds focused round-trip and wire-format tests, NuGet package metadata/readme, and DocFX installation, configuration, support, Native AOT, and migration documentation.