fix: model gidx's family id as a nullable id and revert the serializer patch - #494
Conversation
…r patch #493 patched the serializer so a null sub-packet emits its discriminator with the -1. That was the wrong level: NullableSerializer already carries the splitter into the null branch for a nullable value, and gidx only needed the engine change because the packet was modelled wrong. The server gidx declared FamilyIdentifier as a two-field sub-packet rendering serverId.familyId. The client gidx next to it already declares a plain long FamilyId at the same index, and a capture writes a single id. Flattened to long?, so a null family now serialises as "gidx 1 741328 -1 - 0" through the ordinary nullable path with no serializer change at all. ScnPacket index 9 was missing the SpecialSeparator its three siblings carry, so the boots slot went out as three space-separated fields instead of dotted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 35 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Reverts the serializer change from #493. You were right that it did not belong there, and I merged that PR without applying your review — my mistake.
Why the engine change was the wrong level
NullableSerializeralready establishes the correct contract for a null value:The null branch carries the splitter. #493 hand-rolled that same idea into the sub-packet branch as
$"{discriminator}-1"instead of asking why the field was a sub-packet at all.What the packets actually say
Probing the three shapes, with #493 reverted:
SpecialSeparator = "."990.0.0✅11-1❌ glued990 0 0❌-1✅990✅-1✅Only the nullable scalar is right in both directions — and that is what gidx should have been. The client
GidxPacketsitting next to it already declares:while the server one declared a two-field sub-packet rendering
serverId.familyId. A capture writes a single id. Flattened tolong?,GidxFamilySubPacketdeleted (it had no other consumer, here or in NosCore).Result, with no serializer change:
The second is byte-for-byte the captured line.
Also
ScnPacketindex 9 (BootsInstanceDetails) was missing theSpecialSeparator = "."its three siblings carry, so the boots slot serialised as three space-separated fields where the other three were dotted. The spec comment at the bottom of that file says all four arevnum.rare.upgrade.Still open
sc_n's four equipment slots are genuinely compound — dotted when present, bare-1when absent — and no attribute expresses that today; the first row of the table above is still broken for them. I have not invented a mechanism for it. It only matters once NosCore#2281 actually sendssc_n, so it seems better decided together with that PR than guessed at here.115/115 tests pass.
🤖 Generated with Claude Code