feat/terraria 1.4.5.7/.8 port - #3301
Conversation
All untested claudeslop but this is what I had time to do late last evening and before leaving for work today. Trying to get a first pass at something that might work. If you submit none of it and just use it for reference it won't hurt my feelings.
ProjectileKey.Index is a client-local identity, not the slot in Main.projectile; Projectile.TryLookup resolves it through Projectile.keyToIndex[spawner, index]. Every handler using the raw index was acting on an unrelated projectile. Utils.SearchProjectile now goes through keyToIndex and re-validates the key, replacing the old linear identity scan. HandleProjectileNew mirrors vanilla's Main.projHostile rejection. HandleProjectileKill resolves via ProjectileKey.TryGet so generations are compared, dropping stale keys and owner mismatches. HandleNpcStrike drops strikes whose generation doesn't match the NPC in the slot. TSPlayer.RemoveProjectile now sends the live generation. With the old hardcoded 0 the client allocated a fresh slot instead of clearing the projectile, silently breaking every Bouncer removal.
Bouncer rejects a projectile before vanilla creates it, so the server-side generation lookup in RemoveProjectile can never resolve one - it fell back to 0, and the client then allocated a fresh blank slot while the rejected projectile kept flying. Every banned-projectile removal was affected. Carry the generation from the incoming ProjectileKey through NewProjectileEventArgs and ProjectileKillEventArgs, and add a RemoveProjectile overload that takes it. The two-argument overload keeps the server-side lookup for plugins already calling it.
Vanilla range-checks the id before touching Main.npc; TShock read it straight from the packet and indexed with it, so a crafted id threw. Also corrects the ItemOwner comment: a 1.4.5.7 server ignores that packet entirely, and SSC no longer depends on the slot-400 branch because RestoreCharacter clears IgnoreSSCPackets in a finally block.
Greptile SummaryThis PR ports TShock’s packet handling and related APIs to Terraria 1.4.5.7, especially the new generation-aware projectile and NPC identifiers.
Confidence Score: 4/5The projectile lookup must validate the incoming generation before merge so reused identities cannot update stale server projectiles. The new protocol carries a generation specifically to distinguish reused projectile identities, but SearchProjectile discards it and can resolve a stale keyToIndex entry as the current projectile. Files Needing Attention: TShockAPI/Utils.cs and TShockAPI/GetDataHandlers.cs Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Client sends ProjectileKey] --> B[Extract spawner, identity, generation]
B --> C[SearchProjectile receives spawner and identity]
C --> D[keyToIndex resolves candidate slot]
D --> E{Spawner and identity match?}
E -->|Yes| F[Return slot as existing projectile]
E -->|No| G[Return creation sentinel]
F --> H[Generation is not compared]
H --> I[New generation may update stale projectile]
Reviews (1): Last reviewed commit: "fix: bounds check the projectile index i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (key.Spawner == owner && key.Index == identity) | ||
| return index; | ||
|
|
There was a problem hiding this comment.
When a projectile identity is reused, SearchProjectile validates only its spawner and identity, so a stale keyToIndex entry can classify the new generation as an update to the old projectile. This updates the wrong server projectile and skips creation-specific validation and threshold accounting.
|
Please do a pass to clean up superfluous slop comments, as Particles mentioned in Discord |
whoops, cleaning it up rn |
|
okay builds also clean on my side against TSAPI#283. If you need anything else lmk. |
| var owner = args.Data.ReadInt8(); | ||
| owner = (byte)args.Player.Index; | ||
| var index = TShock.Utils.SearchProjectile(ident, owner); | ||
| // 1.4.5.7: i32 ProjectileKey + kill position replace identity/owner |
There was a problem hiding this comment.
Still more superfluous comments here and in other places. Avoid comments that comment on the current change rather than the static state of the code. We don't care that this packet parsing is for 1.4.5.7 or what the old behavior used to be. After the migration everything is 1.4.5.7.
| } | ||
|
|
||
| /// <summary> | ||
| /// Removes a projectile whose generation is already known, e.g. straight from the packet. |
There was a problem hiding this comment.
Nit: I don't think this is the proper usage of "e.g."
ZakFahey
left a comment
ZakFahey
left a comment
There was a problem hiding this comment.
Approving based just on reading the code - I haven't run and tested it. Somebody else please try and do so.
|
For the kind soul who wants to test this: keep in mind that it will only work when Pryaxis/TSAPI#283 is merged or you pull straight from my fork: https://github.com/bulkinglb/TSAPI/tree/fix/terraria-1457-npc-spawning |
ejfattouch
left a comment
ejfattouch
left a comment
There was a problem hiding this comment.
If you want to also add 1.4.5.8 support, just bump OTAPI to 3.3.14. Was going to open a separate pr for this, but since its just a 1-line change to csproj I don't think it deserves a stacked PR. all the logic in this and Pryaxis/TSAPI#283 don't seem to change with OTAPI 3.3.14, it makes more sense to just fold the bump into this PR.
c3ff9b1 to
8dfbab5
Compare
3 participants
Continues @ZakFahey's first pass in #3299 (their commit is preserved here) with the fixes needed to make 1.4.5.7 actually playable.
Depends on Pryaxis/TSAPI#283 — without it no NPCs spawn at all. Submodule pointer intentionally left on general-devel.
Tested on .NET 9 / linux-arm64 with a vanilla 1.4.5.7 client: connect, play, enemies spawn and sync, projectiles resolve correctly.