Skip to content

A sub-packet's last field is its own, not the parent's - #503

Merged
erwan-joly merged 1 commit into
masterfrom
fix/sub-packet-last-index
Aug 30, 2026
Merged

A sub-packet's last field is its own, not the parent's#503
erwan-joly merged 1 commit into
masterfrom
fix/sub-packet-last-index

Conversation

@erwan-joly

Copy link
Copy Markdown
Contributor

Follow-up to #499, found while removing the manual escape from NosCoreIO/NosCore#2336.

The defect

PacketSerializer passed the parent's maxIndex down when recursing into a sub-packet, so indexAttr.Index == maxIndex was comparing a nested field's number against the outer packet's. Whichever nested field happened to share that number inherited the last-field exemption and kept its spaces.

InPacket ends at index 9. InNonPlayerSubPacket.Name is index 9. So a spaced mate name went out as

in 2 333 2000001 0 0 0 0 0 0 0 3 42 1 0 -1 Joyeux Mouton 0 -1 0 0 ...

which is the same field-splitting #499 set out to fix — it just reached the name by a different route, and #499 did not cover it.

The change

Each sub-packet gets its own maxIndex, so the exemption applies where a field really does end the line and nowhere else. sc_p and sc_n were already correct because their names are plain properties, not nested ones.

Tests

One new, pinning in with a spaced sub-packet name. It fails with the fix stashed and passes with it. 123 total, and the 115 that pin exact wire output are unchanged.

Released as 21.1.1.

PacketSerializer passed the parent's maxIndex down when recursing into a
sub-packet, so whichever nested field happened to share that number inherited
the last-field exemption and kept its spaces. InPacket ends at index 9 and
InNonPlayerSubPacket.Name is index 9, so a spaced mate name went out as

    in 2 333 2000001 ... -1 Joyeux Mouton 0 -1 ...

splitting the field. Each sub-packet now gets its own maxIndex, so the
exemption applies where a field really does end the line and nowhere else.
@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 39 minutes.

View limit details

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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d781e4c-38eb-4994-9ada-a55946344236

📥 Commits

Reviewing files that changed from the base of the PR and between da0c36b and 3b1c9fa.

📒 Files selected for processing (3)
  • src/NosCore.Packets/NosCore.Packets.csproj
  • src/NosCore.Packets/Serializer.cs
  • test/NosCore.Packets.Tests/StringFieldSeparatorTests.cs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@erwan-joly
erwan-joly merged commit 73b1240 into master Aug 30, 2026
2 of 3 checks passed
@erwan-joly
erwan-joly deleted the fix/sub-packet-last-index branch August 30, 2026 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant