Fix batteryless tool battery duplication#2752
Conversation
|
Holding on this briefly. Testing found that the first version fixes the dropped batteryless-tool exploit, but it also affects first-time crafting of battery-powered tools. I am updating the fix so crafted tools still receive their intended default battery while picked-up dropped batteryless tools remain batteryless. |
mindless2831
force-pushed
the
fix-batteryless-tool-duplication
branch
from
3a14d7f to
4c428c9
Compare
|
Updated this PR after additional testing found the first version was too broad. The original one-line guard fixed the dropped batteryless-tool exploit, but it also affected newly crafted battery-powered tools. This has now been corrected. The updated version distinguishes between newly created/generated items and existing known world entities being converted back into inventory:
I retested the exploit path and the crafted-tool regression case, and this should now be good for review again. |
Measurity
left a comment
Measurity
left a comment
There was a problem hiding this comment.
With 2 players, if one drops an item with a battery and the other picks it up. Then no battery is given to the other player.
mindless2831
force-pushed
the
fix-batteryless-tool-duplication
branch
from
4c428c9 to
5bc3443
Compare
|
Updated the PR body and implementation to reflect the final server-side preservation change. The PR now handles the multiplayer pickup case by preserving existing That means the fix now covers:
This should address the prior concern about Player B picking up Player A's dropped battery-powered tool. |
Good catch! Fixed and pushed! |
Measurity
added
Area: spawning
| private void PreserveExistingInstalledBatteries(NitroxId id, InventoryItemEntity pickedUpItem) | ||
| { | ||
| Optional<Entity> existingEntity = entityRegistry.GetEntityById(id); | ||
| if (existingEntity is not { HasValue: true }) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| foreach (InstalledBatteryEntity existingBattery in existingEntity.Value.ChildEntities.OfType<InstalledBatteryEntity>()) | ||
| { | ||
| bool alreadyIncluded = pickedUpItem.ChildEntities | ||
| .OfType<InstalledBatteryEntity>() | ||
| .Any(incomingBattery => | ||
| incomingBattery.Id == existingBattery.Id || | ||
| incomingBattery.ComponentIndex == existingBattery.ComponentIndex); | ||
|
|
||
| if (!alreadyIncluded) | ||
| { | ||
| pickedUpItem.ChildEntities.Add(existingBattery); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unfortunately it's still possible to have buggy behavior.
The client doesn't update the server on the battery of an item properly. Meaning the server is unable to correct it in the highlighted code.
For example:
- Player 1: Fab repair tool
- Player 1: Drop tool
- Player 2: Pickup
- Player 2: Notice no battery in tool
Measurity
removed
the
Status: Waiting for reviews
2 participants
Summary
Fixes an exploit where battery-powered tools regain a new fully charged battery after being dropped and picked back up with no battery installed.
Closes #2668.
Background
Issue #2668 reports that battery-powered tools can duplicate batteries when dropped without an installed battery.
The repro is:
This allows players to repeatedly remove the newly created battery, drop the empty tool again, and generate unlimited fully charged batteries.
Cause
Nitrox already has a dedicated installed-battery sync path. When a battery is installed into an
EnergyMixin, Nitrox represents that installed battery as anInstalledBatteryEntity.The broken behavior came from treating all battery-powered inventory conversion the same way.
There are actually three different cases that need different behavior:
The original exploit happened because an existing dropped batteryless tool could be converted back into an inventory item and incorrectly receive a default installed battery.
A later multiplayer edge case also needed to be handled: when Player A drops a tool with a battery and Player B picks it up, Player B's client may not have that battery represented in the local
EnergyMixin.batterySlot.storedItemat the exact moment the pickup conversion runs. However, the server-side entity state can still contain the correctInstalledBatteryEntity.What changed
This PR now handles battery state in both the client conversion path and the server pickup path.
Client-side inventory conversion
BatteryChildEntityHelper.TryPopulateInstalledBattery(...)now accepts anallowDefaultBatteryflag.When converting newly created/generated items, default battery population is still allowed. This preserves normal crafting behavior.
When converting an already-known existing entity back into inventory, default battery creation is not blindly applied. In that case, the helper only creates a battery child from local slot state when an actual battery is present.
This prevents an existing batteryless dropped tool from being converted into an inventory item with a new default battery.
Server-side pickup preservation
PickupItemPacketProcessornow preserves existingInstalledBatteryEntitychildren from the server's current entity state before replacing the dropped world entity with the picked-upInventoryItemEntity.This protects the multiplayer case where one player drops a tool with an installed battery and another player picks it up. Even if the picking client does not locally serialize that installed battery child, the server carries forward the existing synced installed-battery child state.
Why this approach
This keeps the fix narrow while preserving intended behavior.
The bug was not general battery charge serialization. Tools with installed batteries already had a sync representation through
InstalledBatteryEntity.The missing behavior was distinguishing between:
By allowing default battery creation only where appropriate and preserving existing installed-battery children server-side during pickup, this avoids both the original battery-duplication exploit and the multiplayer pickup regression.
Tested
Tested the original exploit path:
Regression tested crafting:
Regression tested installed batteries:
Regression tested multiplayer pickup: