A spaced mate name reached the client as two fields - #2336
Conversation
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughMate display names now replace spaces with ChangesMate Name Serialization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change keeps mate names containing spaces within a single client packet field across the affected packets. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 mate.NpcMonster.Name.TryGetValue(language, out var localized) | ||
| ? localized | ||
| : mate.NpcMonster.Name[RegionType.EN]; | ||
| return name.Replace(' ', '^'); |
There was a problem hiding this comment.
that should be handled by the serializer already
A mate named "Joyeux Mouton" split its own field and shifted every field after it on in, sc_p and sc_n. It needed no renaming to reach: the fallback when a mate was never renamed is the creature's own name, and plenty carry a space. The serializer owns the substitution as of 21.1.1, so DisplayName returns the name as it is and the packets carry it unchanged. The test that pinned the caret on the packet object now pins it on the wire, where it belongs, across all three packets.
f0e156d to
2082055
Compare
2 participants
Follow-up to #2281, which merged before this landed.
The defect
A mate whose name contains a space reaches the client as two fields. Serialized on master, a mate named
Joyeux Mouton:Every field after the name is shifted by one. It is reachable without renaming anything: the fallback when a mate was never renamed is the creature's own name, and plenty of those carry a space.
Why the serializer does not cover it
Serializer.StringSerializerholds aReplace(splitter, "^"), but the splitter it is handed isindex.SpecialSeparator, which is null for an ordinary property.IsNullOrEmpty(splitter)is then true and the value is returned untouched; the space between fields is added afterwards, byPacketSerializer, and never enters the substitution. So a plain space-separated field keeps its own spaces, and none of the three properties the mate name goes into declares a special separator.Worth a separate look, not changed here: if that
Replaceis meant to guard the field separator rather than a special one, fixing it inNosCore.Packetswould let this line and every other manual substitution in the tree go away.What changed
MateExtensions.DisplayNameputs the substitution back, in one place, so all three generators get it.What was tested
ASpacedMateNameReachesTheClientAsOneFieldAsyncbuilds a realSerializerover the packet assembly and asserts on the serialized line forin,sc_pandsc_n. The existing tests asserted on the packet object, which is why the shift went unnoticed.Joyeux^Mouton.TheCreatureNameIsUsedWhenTheMateWasNeverRenamedAsyncnow expects the escaped form.dotnet build NosCore.slnat 0 warnings,dotnet testgreen with the CI filter (533 in NosCore.GameObject.Tests).Summary by CodeRabbit
Bug Fixes
Tests