Issue 5256: Refactored MekHQ Unit's Gunners to be Set to ensure gunne…

…rs are unique
MegaMek · Dec 4, 2024 · d8c52da · d8c52da
1 parent 3d3fc7c
commit d8c52da
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 16 deletions.
diff --git a/MekHQ/src/mekhq/campaign/unit/Unit.java b/MekHQ/src/mekhq/campaign/unit/Unit.java
@@ -125,7 +125,7 @@ public class Unit implements ITechnology {
     protected int scenarioId;
 
     private List<Person> drivers;
-    private List<Person> gunners;
+    private Set<Person> gunners;
     private List<Person> vesselCrew;
     // Contains unique Id of each Infantry/BA Entity assigned to this unit as
     // marines
@@ -180,7 +180,7 @@ public Unit(Entity en, Campaign c) {
         this.parts = new ArrayList<>();
         this.podSpace = new ArrayList<>();
         this.drivers = new ArrayList<>();
-        this.gunners = new ArrayList<>();
+        this.gunners = new HashSet<>();
         this.vesselCrew = new ArrayList<>();
         forceId = Force.FORCE_NONE;
         scenarioId = Scenario.S_DEFAULT_ID;
@@ -4688,8 +4688,8 @@ public List<Person> getDrivers() {
         return Collections.unmodifiableList(drivers);
     }
 
-    public List<Person> getGunners() {
-        return Collections.unmodifiableList(gunners);
+    public Set<Person> getGunners() {
+        return Collections.unmodifiableSet(gunners);
     }
 
     public List<Person> getVesselCrew() {
@@ -5843,15 +5843,17 @@ public void fixReferences(Campaign campaign) {
                 }
             }
         }
-        for (int ii = gunners.size() - 1; ii >= 0; --ii) {
-            Person gunner = gunners.get(ii);
+        for(Person gunner : gunners){
             if (gunner instanceof UnitPersonRef) {
-                gunners.set(ii, campaign.getPerson(gunner.getId()));
-                if (gunners.get(ii) == null) {
+                gunners.remove(gunner);
+                Person updatedGunner = campaign.getPerson(gunner.getId());
+                if(updatedGunner != null){
+                    gunners.add(updatedGunner);
+                }
+                else{
                     logger.error(
-                            String.format("Unit %s ('%s') references missing gunner %s",
-                                    getId(), getName(), gunner.getId()));
-                    gunners.remove(ii);
+                        String.format("Unit %s ('%s') references missing gunner %s",
+                            getId(), getName(), gunner.getId()));
                 }
             }
         }

diff --git a/MekHQ/src/mekhq/campaign/unit/actions/HirePersonnelUnitAction.java b/MekHQ/src/mekhq/campaign/unit/actions/HirePersonnelUnitAction.java
@@ -29,7 +29,7 @@
 import mekhq.campaign.personnel.generator.DefaultSkillGenerator;
 import mekhq.campaign.unit.Unit;
 
-import java.util.List;
+import java.util.Set;
 
 /**
  * Hires a full complement of personnel for a unit.
@@ -155,7 +155,7 @@ public void execute(Campaign campaign, Unit unit) {
                 && unit.getEntity().getWeaponList().stream()
                         .anyMatch(weapon -> (weapon.getType() instanceof WeaponType)
                                 && (((WeaponType) weapon.getType()).getDamage() == WeaponType.DAMAGE_ARTILLERY))) {
-            final List<Person> gunners = unit.getGunners();
+            final Set<Person> gunners = unit.getGunners();
             if (!gunners.isEmpty() && gunners.stream().noneMatch(person -> person.getSkills().hasSkill(SkillType.S_ARTILLERY))) {
                 new DefaultSkillGenerator(campaign.getRandomSkillPreferences()).generateArtillerySkill(ObjectUtility.getRandomItem(gunners));
             }

diff --git a/MekHQ/unittests/mekhq/campaign/unit/UnitPersonTest.java b/MekHQ/unittests/mekhq/campaign/unit/UnitPersonTest.java
@@ -26,6 +26,7 @@
 import org.junit.jupiter.api.Test;
 
 import java.util.List;
+import java.util.Set;
 import java.util.UUID;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -301,7 +302,7 @@ public void testGunner() {
         verify(unit, times(1)).resetPilotAndEntity();
 
         // Ensure when getting the gunner that it is the same gunner
-        List<Person> gunners = unit.getGunners();
+        Set<Person> gunners = unit.getGunners();
         assertTrue(gunners.contains(mockGunner));
         assertTrue(unit.isGunner(mockGunner));
 

diff --git a/MekHQ/unittests/mekhq/gui/model/UnitTableModelTest.java b/MekHQ/unittests/mekhq/gui/model/UnitTableModelTest.java
@@ -29,7 +29,9 @@
  import org.mockito.junit.jupiter.MockitoExtension;
 
  import java.util.Collections;
+ import java.util.HashSet;
  import java.util.List;
+ import java.util.Set;
 
  import static org.junit.jupiter.api.Assertions.assertEquals;
  import static org.mockito.Mockito.mock;
@@ -66,7 +68,7 @@ private void setCrew(int driverCount, int totalDriverNeeds,
                          boolean hasNavigator, Entity entity,
                          String expected) {
         List<Person> drivers = Collections.nCopies(driverCount, crewMember);
-        List<Person> gunners = Collections.nCopies(gunnerCount, crewMember);
+        Set<Person> gunners = new HashSet<Person>(Collections.nCopies(gunnerCount, crewMember));
         List<Person> crew = Collections.nCopies(crewCount + drivers.size() + gunners.size()
                 + (hasNavigator ? 1 : 0), crewMember);
 
@@ -134,4 +136,4 @@ public void noAssignedCrew() {
                 false, mock(Jumpship.class),
                 "<html><b>Drivers: </b>0/1<br><b>Gunners: </b>0/1<br><b>Crew: </b>0/1<br><b>Navigator: </b>0/1</html>");
     }
-}
+}