Update protocol to 145 #2
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并给出了一些整体性反馈：
QuickStackChestDataSerializer.WriteOverride在写入槽位 ID 之前同时写了ItemCount和SlotIds.Length，而ReadOverride只读取了一个计数值然后按该计数读取若干个 short，因此序列化布局无法往返，需要对齐（要么去掉多余的长度写入，要么在ReadOverride中也读取这一长度）。ExtraSpawnPointDataSerializer.WriteOverride把Count写了两次（一次是Count本身，一次是SpawnPoints.Length），而ReadOverride只读了一个计数字节，这会导致格式不一致并使流反序列化错位；需要调整一侧的实现，使双方在字段顺序和数量上完全一致。InternalTypeSerializers.ArraySerializer.Configure现在会把声明类型和属性名写到控制台，看起来像是遗留的调试日志，在正常使用时会刷屏 stdout；建议移除，或放在一个受调试开关控制的日志里。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- QuickStackChestDataSerializer.WriteOverride writes both ItemCount and SlotIds.Length before the slot IDs, but ReadOverride only reads a single count and then that many shorts, so the serialized layout does not round-trip and should be aligned (either drop the extra length write or mirror it in ReadOverride).
- ExtraSpawnPointDataSerializer.WriteOverride writes Count twice (once as Count and once as SpawnPoints.Length), while ReadOverride only reads a single count byte, so the format is inconsistent and will desynchronize the stream; adjust one side so they agree on the exact sequence of fields.
- InternalTypeSerializers.ArraySerializer.Configure now writes the declaring type and property name to Console, which looks like leftover debug logging and will spam stdout in normal use; consider removing or gating it behind a debug flag.
## Individual Comments
### Comment 1
<location> `TrProtocol/Serializers/QuickStackChestSerrailizer.cs:23-31` </location>
<code_context>
+ data.ChestSlot = br.ReadInt16();
+ return data;
+ }
+ protected override void WriteOverride(BinaryWriter bw, QuickStackChestData t)
+ {
+ bw.Write(t.ItemCount);
+ bw.Write(t.SlotIds.Length);
+ foreach (short slotId in t.SlotIds)
+ {
+ bw.Write(slotId);
+ }
+ bw.Write(t.ChestSlot);
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** QuickStackChestData write format does not match the read format, which will corrupt serialization.
`ReadOverride` expects `[ItemCount:int] [ItemCount shorts for SlotIds] [ChestSlot:short]`, but `WriteOverride` writes `[ItemCount:int] [SlotIds.Length:int] [SlotIds...] [ChestSlot]`. The extra `SlotIds.Length` causes the reader to treat it as the first slot ID and misaligns all subsequent data. Make the formats symmetric by either removing `bw.Write(t.SlotIds.Length);` or updating the reader to consume this length and use it for array allocation.
</issue_to_address>
### Comment 2
<location> `TrProtocol/Packets/Modules/TagEffectStateNetModule.cs:35-39` </location>
<code_context>
+ return module;
+ }
+
+ public readonly void Serialize(BinaryWriter bw)
+ {
+ bw.Write(KillCount);
</code_context>
<issue_to_address>
**issue (bug_risk):** TagEffectFullStateModule.Serialize assumes ProcTimeLeftOnNPCs is non-null, but Deserialize may leave it null.
In `Deserialize`, `ProcTimeLeftOnNPCs` is only created when there’s remaining stream data; otherwise it stays null. `Serialize` then always passes it to `WriteSparseNPCTimeArray`, causing a possible null reference. Either always allocate a 200‑length array in `Deserialize`, or add a null check in `Serialize` (and corresponding write) so missing proc data round‑trips safely.
</issue_to_address>
### Comment 3
<location> `TrProtocol/Serializers/InternalTypeSerializers.cs:91-92` </location>
<code_context>
public IConfigurable Configure(PropertyInfo prop, string version)
{
+ Console.WriteLine(prop.DeclaringType);
+ Console.WriteLine(prop.Name);
if (elementSerializer is IConfigurable conf)
conf.Configure(prop, version);
</code_context>
<issue_to_address>
**suggestion (performance):** Console.WriteLine calls in serializer configuration will cause noisy stdout logging in production.
Because `Configure` is called for every property, these `Console.WriteLine` calls will run frequently, cluttering stdout for library consumers and slowing down serialization. If they’re only for debugging, please remove them or switch to a configurable logger guarded by a debug/trace flag.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据这些反馈改进后续的 review。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- QuickStackChestDataSerializer.WriteOverride writes both ItemCount and SlotIds.Length before the slot IDs, but ReadOverride only reads a single count and then that many shorts, so the serialized layout does not round-trip and should be aligned (either drop the extra length write or mirror it in ReadOverride).
- ExtraSpawnPointDataSerializer.WriteOverride writes Count twice (once as Count and once as SpawnPoints.Length), while ReadOverride only reads a single count byte, so the format is inconsistent and will desynchronize the stream; adjust one side so they agree on the exact sequence of fields.
- InternalTypeSerializers.ArraySerializer.Configure now writes the declaring type and property name to Console, which looks like leftover debug logging and will spam stdout in normal use; consider removing or gating it behind a debug flag.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- QuickStackChestDataSerializer.WriteOverride writes both ItemCount and SlotIds.Length before the slot IDs, but ReadOverride only reads a single count and then that many shorts, so the serialized layout does not round-trip and should be aligned (either drop the extra length write or mirror it in ReadOverride).
- ExtraSpawnPointDataSerializer.WriteOverride writes Count twice (once as Count and once as SpawnPoints.Length), while ReadOverride only reads a single count byte, so the format is inconsistent and will desynchronize the stream; adjust one side so they agree on the exact sequence of fields.
- InternalTypeSerializers.ArraySerializer.Configure now writes the declaring type and property name to Console, which looks like leftover debug logging and will spam stdout in normal use; consider removing or gating it behind a debug flag.
## Individual Comments
### Comment 1
<location> `TrProtocol/Serializers/QuickStackChestSerrailizer.cs:23-31` </location>
<code_context>
+ data.ChestSlot = br.ReadInt16();
+ return data;
+ }
+ protected override void WriteOverride(BinaryWriter bw, QuickStackChestData t)
+ {
+ bw.Write(t.ItemCount);
+ bw.Write(t.SlotIds.Length);
+ foreach (short slotId in t.SlotIds)
+ {
+ bw.Write(slotId);
+ }
+ bw.Write(t.ChestSlot);
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** QuickStackChestData write format does not match the read format, which will corrupt serialization.
`ReadOverride` expects `[ItemCount:int] [ItemCount shorts for SlotIds] [ChestSlot:short]`, but `WriteOverride` writes `[ItemCount:int] [SlotIds.Length:int] [SlotIds...] [ChestSlot]`. The extra `SlotIds.Length` causes the reader to treat it as the first slot ID and misaligns all subsequent data. Make the formats symmetric by either removing `bw.Write(t.SlotIds.Length);` or updating the reader to consume this length and use it for array allocation.
</issue_to_address>
### Comment 2
<location> `TrProtocol/Packets/Modules/TagEffectStateNetModule.cs:35-39` </location>
<code_context>
+ return module;
+ }
+
+ public readonly void Serialize(BinaryWriter bw)
+ {
+ bw.Write(KillCount);
</code_context>
<issue_to_address>
**issue (bug_risk):** TagEffectFullStateModule.Serialize assumes ProcTimeLeftOnNPCs is non-null, but Deserialize may leave it null.
In `Deserialize`, `ProcTimeLeftOnNPCs` is only created when there’s remaining stream data; otherwise it stays null. `Serialize` then always passes it to `WriteSparseNPCTimeArray`, causing a possible null reference. Either always allocate a 200‑length array in `Deserialize`, or add a null check in `Serialize` (and corresponding write) so missing proc data round‑trips safely.
</issue_to_address>
### Comment 3
<location> `TrProtocol/Serializers/InternalTypeSerializers.cs:91-92` </location>
<code_context>
public IConfigurable Configure(PropertyInfo prop, string version)
{
+ Console.WriteLine(prop.DeclaringType);
+ Console.WriteLine(prop.Name);
if (elementSerializer is IConfigurable conf)
conf.Configure(prop, version);
</code_context>
<issue_to_address>
**suggestion (performance):** Console.WriteLine calls in serializer configuration will cause noisy stdout logging in production.
Because `Configure` is called for every property, these `Console.WriteLine` calls will run frequently, cluttering stdout for library consumers and slowing down serialization. If they’re only for debugging, please remove them or switch to a configurable logger guarded by a debug/trace flag.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+23
to
+31
| protected override void WriteOverride(BinaryWriter bw, QuickStackChestData t) | ||
| { | ||
| bw.Write(t.ItemCount); | ||
| bw.Write(t.SlotIds.Length); | ||
| foreach (short slotId in t.SlotIds) | ||
| { | ||
| bw.Write(slotId); | ||
| } | ||
| bw.Write(t.ChestSlot); |
There was a problem hiding this comment.
issue (bug_risk): QuickStackChestData 的写入格式与读取格式不匹配，会破坏序列化结果。
ReadOverride 期望的格式是 [ItemCount:int] [ItemCount 个 SlotIds 的 short] [ChestSlot:short]，但 WriteOverride 写的是 [ItemCount:int] [SlotIds.Length:int] [SlotIds...] [ChestSlot]。多出来的 SlotIds.Length 会被读取端当成第一个槽位 ID，从而导致后续数据全部错位。请让读写格式保持对称：要么移除 bw.Write(t.SlotIds.Length);，要么更新读取逻辑，先读入该长度并据此分配数组。
Original comment in English
issue (bug_risk): QuickStackChestData write format does not match the read format, which will corrupt serialization.
ReadOverride expects [ItemCount:int] [ItemCount shorts for SlotIds] [ChestSlot:short], but WriteOverride writes [ItemCount:int] [SlotIds.Length:int] [SlotIds...] [ChestSlot]. The extra SlotIds.Length causes the reader to treat it as the first slot ID and misaligns all subsequent data. Make the formats symmetric by either removing bw.Write(t.SlotIds.Length); or updating the reader to consume this length and use it for array allocation.
Comment on lines
+35
to
+39
| public readonly void Serialize(BinaryWriter bw) | ||
| { | ||
| bw.Write(Type); | ||
| WriteSparseNPCTimeArray(bw, TimeLeftOnNPCs); | ||
| WriteSparseNPCTimeArray(bw, ProcTimeLeftOnNPCs); |
There was a problem hiding this comment.
issue (bug_risk): TagEffectFullStateModule.Serialize 假定 ProcTimeLeftOnNPCs 一定非空，但 Deserialize 可能会让它保持为 null。
在 Deserialize 中，只有当流中还有剩余数据时才会创建 ProcTimeLeftOnNPCs；否则它保持为 null。而 Serialize 总是把它传给 WriteSparseNPCTimeArray，这可能导致空引用异常。可以考虑：要么在 Deserialize 中始终分配一个长度为 200 的数组，要么在 Serialize 中加入空值检查（以及相应的写入逻辑），以便在缺少 proc 数据时仍然能够安全往返序列化。
Original comment in English
issue (bug_risk): TagEffectFullStateModule.Serialize assumes ProcTimeLeftOnNPCs is non-null, but Deserialize may leave it null.
In Deserialize, ProcTimeLeftOnNPCs is only created when there’s remaining stream data; otherwise it stays null. Serialize then always passes it to WriteSparseNPCTimeArray, causing a possible null reference. Either always allocate a 200‑length array in Deserialize, or add a null check in Serialize (and corresponding write) so missing proc data round‑trips safely.
Comment on lines
+91
to
+92
| Console.WriteLine(prop.DeclaringType); | ||
| Console.WriteLine(prop.Name); |
There was a problem hiding this comment.
suggestion (performance): 在序列化器配置中调用 Console.WriteLine 会在生产环境产生大量 stdout 噪声。
由于 Configure 会对每个属性调用，这些 Console.WriteLine 会被频繁执行，不但会为库的使用者制造很多 stdout 日志，还会拖慢序列化过程。如果这些输出只是为了调试，建议将其移除，或改用受调试/追踪开关控制的可配置日志系统。
Original comment in English
suggestion (performance): Console.WriteLine calls in serializer configuration will cause noisy stdout logging in production.
Because Configure is called for every property, these Console.WriteLine calls will run frequently, cluttering stdout for library consumers and slowing down serialization. If they’re only for debugging, please remove them or switch to a configurable logger guarded by a debug/trace flag.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
更新网络协议实现以匹配协议版本 145/315，弃用旧的消息 ID，新增消息/NetModule 类型和数据包，并使数据包字段和序列化器与最新的 Terraria 服务器/客户端行为保持一致。
新功能：
漏洞修复：
LoadNetModule而不是NetModules，以及更新后的枚举值），以匹配更新后的协议。增强改进：
NetLiquidModule或NetTextModule），并文档化这些被弃用的行为。Original summary in English
Summary by Sourcery
Update the network protocol implementation to match protocol 145/315, deprecating old message IDs, adding new message/NetModule types and packets, and aligning packet fields and serializers with the latest Terraria server/client behavior.
New Features:
Bug Fixes:
Enhancements: