fix: a null sub-packet loses its separating space - #493
Conversation
A sub-packet serialized from null returns "-1" without the leading separator the non-null path
gets from its discriminator, so the -1 ends up glued to the field before it and the client
cannot split the packet into fields at all:
sc_n 1 319 26719 50 1000 1536-1-1-1 -1 0 ...
instead of
sc_n 1 319 26719 50 1000 1536 -1 -1 -1 0 ...
It shows anywhere a sub-packet property can be null, which today means sc_n's four equipment
slots and gidx's family identifier.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8351771 to
fba5074
Compare
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe test fixture registers ChangesSerialization test coverage
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
| return Expression.Condition( | ||
| Expression.Equal(specificTypeExpression, Expression.Constant(null, typeof(object))), | ||
| Expression.Constant(indexAttr.IsOptional ? null : "-1", typeof(object)), | ||
| Expression.Constant(indexAttr.IsOptional ? null : $"{discriminator}-1", typeof(object)), |
There was a problem hiding this comment.
I don't think it's supposed to be there we have attribute to handle those edge case
| incrementExpr = Expression.Constant(!isFromList || !isOptionalSerie); | ||
| } | ||
|
|
||
| // A null sub-packet is written as -1, and it has to carry the same leading separator |
There was a problem hiding this comment.
don't need this comment
The new case built an ScnPacket but the serializer in this fixture only knows the types it is handed, so Serialize threw KeyNotFoundException before it could assert anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Comment removed. On the attribute — I would rather you point me at the one you mean than guess, because I could not find it. Here is what I traced, in case it changes the answer. The leading space before a sub-packet does not come from the property's own splitter. For a property with index.SpecialSeparator != null
? Expression.Constant(indexAttr.SpecialSeparator ?? string.Empty, typeof(object))
: (Expression)Expression.Convert(propertySplitter, typeof(object));and the space is put back further down by handing the sub-packet a discriminator of if (header == null && indexAttr.SpecialSeparator != null)
{
header = " ";
}The null branch returns before that discriminator is ever used, which is why the space disappears only when the value is null. So as far as I can see the current mechanism is Separately, and independent of this: And a data point that may bear on both: a capture writes all four of those slots as a bare |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for merging. One consequence worth knowing: A character with a family still does not, because |
…r patch (#494) #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>
|
Thanks for #494 — flattening the family id is right, and I have verified it against the capture on 21.0.0. Both cases reproduce exactly now: Bumping NosCore to 21.0.0 in NosCoreIO/NosCore#2286. One thing to flag rather than reopen, since you reverted the serializer change deliberately: The real server writes an empty slot as a bare It costs nothing today — nothing creates a partner yet, so the branch is unreachable — and I am not going to push on it. But if a partner ever gets equipment, that is where it will surface, and the same shape applies to any future packet with an optional sub-packet. Happy to leave it closed if you would rather solve it another way when it matters. |
2 participants
A sub-packet serialized from
nullreturns"-1"without the leading separator that the non-null path gets from its discriminator. The-1ends up glued to the field before it, and the client cannot split the packet into fields at all.It shows up anywhere a sub-packet property can be null. Today that is
sc_n's four equipment slots andgidx's family identifier — I hit it building both (NosCoreIO/NosCore#2281, NosCoreIO/NosCore#2283).One-line fix in
Serializer.PacketSerializer, plus a regression test onsc_n.Two test failures on master are pre-existing and unrelated —
DocumentationTestrefuses CRLF checkouts on Windows; they fail identically with this change reverted.While I was here:
StringSerializeralready replaces the separator with^for every non-final string field, so callers do not need to do it by hand. I had been doing exactly that in NosCore and you were right to flag it — I am removing it there, no change needed here.One thing I would like your opinion on rather than change unilaterally:
GidxPacket.FamilyIdentifieris modelled as aserverId.familyIdcompound sub-packet, but a packet capture writes that field as a single family id in all 670gidxlines — not one contains a dot — and-1when the character has no family. As modelled it can produce neither. Would you like a follow-up flattening it to a nullable id, or is the compound deliberate for something cross-server?🤖 Generated with Claude Code
Summary by CodeRabbit
-1placeholder during serialization.null.