Fix: Fixed Ammo Not Stacking in the Warehouse #6816
Conversation
### Dev Notes We had a couple of lines of code that were trying to specially handle ammo. However, we were already handling ammo later in the process. The two handlers were incompatible, which resulted in ammo refusing to stack.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6816 +/- ##
============================================
- Coverage 11.46% 11.44% -0.03%
+ Complexity 6508 6507 -1
============================================
Files 1098 1099 +1
Lines 140812 140909 +97
Branches 21831 21841 +10
============================================
- Hits 16150 16132 -18
- Misses 122995 123104 +109
- Partials 1667 1673 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -263,13 +271,52 @@ private Part mergePartWithExisting(Part part) { | |||
| * @return The matching spare part or null if none were found. | |||
| */ | |||
| public @Nullable Part checkForExistingSparePart(Part part) { | |||
| Objects.requireNonNull(part); | |||
| if (part == null) { | |||
| logger.error("checkForExistingSparePart(Part): Part is null"); | |||
There was a problem hiding this comment.
This is not an error, it's a debug, Maybe an info, but it will show up in the log as an error and it will mean nothing, it won't have any kind of information, wont show an ID, nothing, so its an error log that will exist but no one will be able to do anything to act upon this error being logged.
| if (part == null) { | ||
| logger.error("checkForExistingSparePart(Part, boolean): Part is null"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Check for null first unless checkForExistingSparePart accepts @nullable.
Also, this log is another instance of an error state that isn't really an error, since there is nothing that can be done about it other than say "something happened". Either you raise that as a special exception so somewhere "upstream" you can manually handle the error, or you just accept that if you receive a null you will return a null. In either case, you can remove this error log.
There was a problem hiding this comment.
Surely we should be logging unexpected null? Otherwise we end up in situations where erroneous nulls are being swallowed silently with nothing in the logs to show it happened. As occurred in 50.05's dev cycle which nearly led us to shipping 50.05 with completely broken OpFor generation.
There was a problem hiding this comment.
If you log null you dont get any meaningful information other than that a null arrived there, there is no ID that generated the null, no entity, nothing, meaning it is useless from a bug-fixing standpoint. If you want to log that something went wrong, sure, add a warning with something like expected part ID {} but received null, which won't give you any information unless you had an info log prior to that which gave something like logger.info("Looking at unit {}", unit);
There was a problem hiding this comment.
What I ended up doing has logging a handled NPE, so that we'd get the stack dump which will tell us where the error was coming from.
| if ((part instanceof AmmoBin) && (null == part.getUnit())) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Are we 100% certain that this piece of code is unnecessary? I won't judge, just want a second confirmation from you @IllianiBird that this is 100% as intended.
There was a problem hiding this comment.
It shouldn't be necessary, but you know what? Let's add it back. If it's not necessary it won't do any harm, and if it is somehow necessary (despite being handled elsewhere in the pipeline) it's sitting pretty.
…cking # Conflicts: # MekHQ/src/mekhq/campaign/Warehouse.java
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the ammo stacking issue by modifying the merging logic in the Warehouse, ensuring that spare parts are correctly compared based on their "brand new" state. Key changes include:
- Replacing reference equality with an equals check for merged parts in Warehouse.addPart.
- Adding an overloaded checkForExistingSparePart method that considers newness.
- Updating copyright headers in both Warehouse and Quartermaster.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| MekHQ/src/mekhq/campaign/Warehouse.java | Updated merging and spare part checking logic to fix ammo stacking issues. |
| MekHQ/src/mekhq/campaign/Quartermaster.java | Updated copyright header. |
Comments suppressed due to low confidence (1)
MekHQ/src/mekhq/campaign/Warehouse.java:89
- Replacing the intentional reference equality check with an equals comparison may lead to unexpected behavior if the equals method does not match the intended identity check. Verify that this change correctly identifies merged parts without side effects.
if (!mergedPart.equals(part)) {
Dev Notes
This issue was caused by the code that checks whether an item already exists in the warehouse, prior to attempting to stack the parts. What was happening is we checked the warehouse and if the first eligible item we found didn't have the same newness as the part we were comparing MekHQ would mistakenly think there were no eligible items. This resulted in the part (usually ammo) not stacking.
We also had a couple of lines of code that were trying to specially handle ammo. However, we were already handling ammo later in the process. So I removed it.