Buildings - #8946
Open
exeea wants to merge 17 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds mobile buildings and MegaMekLab construction support, including movement, cargo, environmental effects, walls, and validation.
Changes:
- Adds mobile-structure movement, linkage, naval, collision, and cargo rules.
- Extends building construction, environmental damage, collapse, flight decks, and walls.
- Adds broad regression-test coverage.
File summaries
| File | Description |
|---|---|
| megamek/unittests/megamek/server/totalWarfare/EngineExplosionRatingTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/server/totalWarfare/BuildingWeaponUnjamTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/server/totalWarfare/BuildingEntryMovementTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/verifier/TestSupportVehicleTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/verifier/TestInfantrySquadCountTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/verifier/TestAdvancedAerospaceTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/units/BuildingGmStateEditTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/options/GameOptionsTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/loaders/BLKStructureFileTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/cost/CostCalculatorReportTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/ComputeTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/ComputeECMTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/bays/InfantryBayPersonnelTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/common/actions/compute/ComputeToHitTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/client/ui/tileset/MekTilesetBuildingTest.java | Updated as part of this pull request. |
| megamek/unittests/megamek/client/ui/panels/GameOptionsPaneTest.java | Updated as part of this pull request. |
| megamek/testresources/megamek/common/units/MekBayMobileStructure.blk | Updated as part of this pull request. |
| megamek/testresources/megamek/common/units/MekBayMobilePortal.blk | Updated as part of this pull request. |
| megamek/src/megamek/server/totalWarfare/WeaponMalfunctionRepairHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/server/totalWarfare/TWPhasePreparationManager.java | Updated as part of this pull request. |
| megamek/src/megamek/server/totalWarfare/TWPhaseEndManager.java | Updated as part of this pull request. |
| megamek/src/megamek/server/totalWarfare/TWDamageManager.java | Updated as part of this pull request. |
| megamek/src/megamek/server/totalWarfare/DeploymentProcessor.java | Updated as part of this pull request. |
| megamek/src/megamek/server/totalWarfare/BuildingEditHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/server/IndustrialElevatorProcessor.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/infantry/InfantryWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/infantry/InfantryInfernoSRMHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/infantry/InfantryDisposableWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/VariableSpeedPulseLaserWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/UltraWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/ThunderBoltWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/StreakHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/srm/SRMTandemChargeHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/srm/SRMInfernoHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/srm/SRMHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/srm/SRMFragHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/srm/SRMDeadFireHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/srm/SRMAntiTSMHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/RifleWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/RapidFireACWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/PulseLaserWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/prototype/PrototypeLBXHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/PPCHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/PopUpMineLauncherHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/plasma/PlasmaRifleHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/plasma/PlasmaCannonHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/NarcExplosiveHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/MPodHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/MissileWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/MissileMineClearanceHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/MissileBayWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/MGHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/MGAWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/mekMortar/MekMortarHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/mekMortar/MekMortarAntiPersonnelHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/mekMortar/MekMortarAirburstHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/lrm/StreakLRMHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/lrm/LRMSwarmHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/lrm/LRMHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/lrm/LRMFragHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/lrm/LRMAntiTSMHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/LBXHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/HyperLaserHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/HAGWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/GRHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/FlamerHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/EnergyWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/CLLBXPrototypeHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/CLIATMHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/ChemicalLaserHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/capitalMissile/CapitalMissileHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/capitalMissile/CapitalMissileBearingsOnlyHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/capitalMissile/CapitalMissileBayHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/CapitalLaserBayOrbitalBombardmentHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/BPodHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/BayWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/AttackHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/ATMHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/artillery/ArtilleryWeaponDistantHomingHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/artillery/ArtilleryWeaponDistantFireHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/artillery/ArtilleryCannonWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/artillery/ArtilleryBayWeaponDistantHomingHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/artillery/ArtilleryBayWeaponDistantFireHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/AR10Handler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/ac/ACWeaponHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/handlers/ac/ACAPHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/battleArmor/BAMGHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/weapons/battleArmor/BALBXHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/common/verifier/TestEntity.java | Updated as part of this pull request. |
| megamek/src/megamek/common/verifier/TestBattleArmor.java | Updated as part of this pull request. |
| megamek/src/megamek/common/verifier/TestAdvancedAerospace.java | Updated as part of this pull request. |
| megamek/src/megamek/common/verifier/BayData.java | Updated as part of this pull request. |
| megamek/src/megamek/common/util/SerializationHelper.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/Targetable.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/MobileStructureNavalRules.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/MobileStructureBayLaunch.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/Mek.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/IAero.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/Dropship.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/DamageEditSpec.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/ConvInfantry.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/ConstructionUtil.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/BuildingTarget.java | Updated as part of this pull request. |
| megamek/src/megamek/common/units/BuildingFoundationRules.java | Updated as part of this pull request. |
| megamek/src/megamek/common/ToHitData.java | Updated as part of this pull request. |
| megamek/src/megamek/common/planetaryConditions/PlanetaryConditions.java | Updated as part of this pull request. |
| megamek/src/megamek/common/options/OptionsConstants.java | Updated as part of this pull request. |
| megamek/src/megamek/common/options/GameOptions.java | Updated as part of this pull request. |
| megamek/src/megamek/common/net/enums/PacketCommand.java | Updated as part of this pull request. |
| megamek/src/megamek/common/net/connections/DataStreamConnection.java | Updated as part of this pull request. |
| megamek/src/megamek/common/net/connections/AbstractConnection.java | Updated as part of this pull request. |
| megamek/src/megamek/common/moves/WallStep.java | Updated as part of this pull request. |
| megamek/src/megamek/common/moves/TurnStep.java | Updated as part of this pull request. |
| megamek/src/megamek/common/moves/PhasePassSelector.java | Updated as part of this pull request. |
| megamek/src/megamek/common/moves/MovePath.java | Updated as part of this pull request. |
| megamek/src/megamek/common/moves/MobileStructureSupport.java | Updated as part of this pull request. |
| megamek/src/megamek/common/loaders/MekFileParser.java | Updated as part of this pull request. |
| megamek/src/megamek/common/HitData.java | Updated as part of this pull request. |
| megamek/src/megamek/common/event/GameCFREvent.java | Updated as part of this pull request. |
| megamek/src/megamek/common/equipment/Sensor.java | Updated as part of this pull request. |
| megamek/src/megamek/common/equipment/PowerGeneratorType.java | Updated as part of this pull request. |
| megamek/src/megamek/common/equipment/EquipmentType.java | Updated as part of this pull request. |
| megamek/src/megamek/common/equipment/enums/StructureEngine.java | Updated as part of this pull request. |
| megamek/src/megamek/common/equipment/Engine.java | Updated as part of this pull request. |
| megamek/src/megamek/common/enums/MoveStepType.java | Updated as part of this pull request. |
| megamek/src/megamek/common/enums/BuildingType.java | Updated as part of this pull request. |
| megamek/src/megamek/common/cost/CostCalculator.java | Updated as part of this pull request. |
| megamek/src/megamek/common/compute/ComputeECM.java | Updated as part of this pull request. |
| megamek/src/megamek/common/board/CubeCoords.java | Updated as part of this pull request. |
| megamek/src/megamek/common/board/AllowedDeploymentHelper.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/SuperHeavyVehicleBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/SteerageQuartersCargoBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/StandardSeatCargoBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/SmallCraftBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/SecondClassQuartersCargoBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/RefrigeratedCargoBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/ProtoMekBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/MekBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/LivestockCargoBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/LiquidCargoBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/LightVehicleBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/InsulatedCargoBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/InfantryBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/HeavyVehicleBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/FirstClassQuartersCargoBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/CrewQuartersCargoBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/CargoBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/Bay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/BattleArmorBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/bays/ASFBay.java | Updated as part of this pull request. |
| megamek/src/megamek/common/battleValue/AbstractBuildingEntityBVCalculator.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/UnloadStrandedAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/ThrashAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/PushAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/PunchAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/PhysicalAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/LayExplosivesAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/KickAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/JumpJetAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/DfaAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/compute/ComputeToHitIsImpossible.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/compute/ComputeToHit.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/compute/ComputeTerrainMods.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/compute/ComputeEnvironmentalToHitMods.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/compute/ComputeAbilityMods.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/ClubAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/ChargeAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/BrushOffAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/AirMekRamAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/common/actions/AbstractAttackAction.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/tileset/MekTileset.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/SharedUtility.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/panels/phaseDisplay/FiringDisplay.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/panels/phaseDisplay/DeploymentDisplay.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/panels/phaseDisplay/commands/MoveCommand.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/panels/phaseDisplay/AimedShotHandler.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/panels/GameOptionsPresentation.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/dialogs/unitEditor/UnitDamageSpecBuilder.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/dialogs/unitEditor/UnitDamagePanelBuilder.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/dialogs/unitEditor/UnitDamageControls.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/clientGUI/ClientGUI.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/clientGUI/boardview/sprite/EntitySprite.java | Updated as part of this pull request. |
| megamek/src/megamek/client/ui/clientGUI/boardview/BoardView.java | Updated as part of this pull request. |
| megamek/src/megamek/client/Client.java | Updated as part of this pull request. |
| megamek/src/megamek/client/bot/princess/FireControl.java | Updated as part of this pull request. |
| megamek/src/megamek/client/bot/BotClient.java | Updated as part of this pull request. |
| megamek/resources/megamek/common/report-messages.properties | Updated as part of this pull request. |
| megamek/resources/megamek/common/options/messages.properties | Updated as part of this pull request. |
| megamek/resources/megamek/common/messages.properties | Updated as part of this pull request. |
| megamek/resources/megamek/client/messages.properties | Updated as part of this pull request. |
Review details
Suppressed comments (8)
megamek/src/megamek/common/bays/CargoBay.java:75
Bay.canLoad(Entity)now passes the computed usable-door count into this overload, but this implementation ignores it and therefore still returns true when every physical bay door is unusable (for example after all authored doors are damaged or blocked). Loading then bypasses the building/mobile-structure door restriction. Preserve the infantry/capacity check and also require an available door for this turn, as the base implementation does.
megamek/src/megamek/common/cost/BuildingCostCalculator.java:62- For a mobile structure with
hex_heights, this cost loop multiplies every footprint hex by the overall height. A shorter hex is therefore charged for floors it does not have, even though the verifier explicitly permits per-hex heights; use each hex'sgetHeight(...)for the structure-cost multiplier.
megamek/src/megamek/common/loaders/BLKStructureFile.java:130 - The new per-hex-height format is accepted here, but the loader later iterates every level up to the overall structure height and calls
loadEquipmentfor it. A BLK file can therefore place equipment in levels that do not exist in a shorter footprint hex; the equipment-loading loop must skip levels at or abovegetHeight(coords)(or reject them).
megamek/src/megamek/common/units/BuildingConstruction.java:180 - When a mobile structure uses
hex_heights, this branch still gives a shorter hex the full structure-height carrying capacity. The verifier calls this per occupied hex, so equipment in a two-level hex of a four-level structure can be accepted as if it had four floors; use the actualgetHeight(hex)here, as the static-building branch already does.
megamek/src/megamek/common/units/MobileStructureCargoRules.java:141 InfantryBay,BattleArmorBay, andCargoBayexplicitly allow infantry/battle-armor unloading based on adjacency even when the bay's door count is exhausted, but this projected-pose check appliesusableDoorsto every passenger before it builds the exits. A mobile structure carrying infantry therefore cannot unload after its physical/legacy door count reaches zero, unlike the transporter contract used elsewhere. Apply the door-count limit only to non-infantry passengers (while still letting authored-door filtering decide whether a physical exit exists).
megamek/src/megamek/common/units/MobileStructureCargoRules.java:201resultis keyed only by the outside coordinate, butExitalso carries elevation, deck elevation, and the authored door. Hangar validation permits multiple doors on one exterior edge, including doors on different floors, soputIfAbsentdiscards one valid exit and prevents passengers from using that floor's door. Preserve distinct exits by floor/door identity rather than deduplicating solely byCoords.
megamek/src/megamek/common/units/MobileStructurePortalRules.java:142- The tunnel-matching code deliberately chooses the centered lateral footprint hex as the entrance, but this lookup uses
closed.getFirst(). The authored coordinate order is not the centered entrance for every valid multi-hex portal, so deployment can miss a matching complex simply because that complex is not behind the first list element. Select the same centered lateral hex used bytunnelEquipmentMatchesbefore translating rearward.
megamek/src/megamek/common/verifier/TestBuilding.java:412 - The per-hex heavy-weapon limit also uses the structure-wide
height, so a shorterhex_heightsfootprint hex receives the same weapon allowance as a full-height hex. Since this check is performed per hex, calculate the Fortress/Castles Brian limit fromstructure.getHeight(hex)instead.
- Files reviewed: 229/283 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| static boolean isBuildingType(int type) { | ||
| return type == TYPE_BUILDING || isWallType(type); |
Comment on lines
+316
to
+317
| addReport(gameManager.damageBuilding(unit, damage, "demolishing a building", unit.relativeToBoard(entering), | ||
| 0, null, false)); |
IllianiBird
requested changes
Sep 11, 2026
IllianiBird
left a comment
IllianiBird
left a comment
Collaborator
There was a problem hiding this comment.
For the most part this looks good. I reviewed it in the same way I would a mhq PR, so some of my suggestions won't match legacy mm code style. No abbreviated or single letter variable names, for example. That's a big one because it reduces code clarity.
I also spotted a lot of hardcoded player-facing Strings. Those need to be nixed.
My two main concerns however revolving around performance and code cleanliness. I noticed a lot of streams being used throughout this PR and I don't believe a stream is a good call in most of them.
Streams are surprisingly more expensive that folks realize and unless you're working with thousands of objects a traditional loop is more performative. Having a stream within a stream is a big no-no. Furthermore, I really don't think we should use streams if we're performing more than one or two actions on the array. Once you start chaining actions the stream can quickly get out of hand.
The other thing that had a really bad code smell was the large number of loops nested within loops nested within loops. Nesting a loop within a loop always makes me second guess the design. When we're hitting three loops deep that tells me we might need to reconsider our approach.
As always, I generally only highlight a handful of instances where I see a particular individual issue. So please review your PR for other instances where a similar problems exist but weren't flagged.
| Graphics2D g = (Graphics2D) graphics.create(); | ||
| double[] x = { .25, .75, 1, .75, .25, 0 }; | ||
| double[] y = { 0, 0, .5, 1, 1, .5 }; | ||
| for (var segment : megamek.common.units.WallRules.segments(game, boardId)) { |
Collaborator
There was a problem hiding this comment.
I'd prefer strong typing. I know var is a more modern style, but we get a lot of inexperienced contributors and strong typing is less likely to result in dumbness
|
|
||
| /** Authored walls occupy actual hexsides; an ordinary whole-hex building tile would misrepresent open sides. */ | ||
| private void drawWallSegments(Graphics2D graphics) { | ||
| Graphics2D g = (Graphics2D) graphics.create(); |
Collaborator
There was a problem hiding this comment.
We're trying to move away from single letter or abbreviated variable names to improve code clarity.
| } | ||
|
|
||
| static JMenu buildingDoorsMenu(Game game, Board board, Coords coords, Client client) { | ||
| JMenu menu = new JMenu("Building doors"); |
Collaborator
There was a problem hiding this comment.
Hardcoded player facing String
| if (!game.getPhase().isEnd()) { | ||
| return menu; | ||
| } | ||
| String[] directions = { "N", "NE", "SE", "S", "SW", "NW" }; |
Collaborator
There was a problem hiding this comment.
And here, I won't mark any others but we shouldn't be hardcoding player facing strings
Comment on lines
+228
to
+231
| for (IBuilding candidate : board.getBuildingsAt(coords)) { | ||
| if (!(candidate instanceof AbstractBuildingEntity building)) { continue; } | ||
| var doors = building.getDesign().getDoors(); | ||
| for (int index = 0; index < doors.size(); index++) { |
Collaborator
There was a problem hiding this comment.
Is there any way to make this n(1)?
Comment on lines
+95
to
+98
|
|
||
|
|
||
|
|
||
|
|
| } | ||
| public double capacity() { | ||
| if (!landingDeck()) { return 200; } | ||
| return 1000.0 * hexes.size(); |
Collaborator
There was a problem hiding this comment.
If this has special meaning please store it as a const, or at least define it in the method so we're not using a magic number
| return false; | ||
| } | ||
| /** Local surface-relative elevation of an authorized underground passage's floor, or null outside it. */ | ||
| public static Integer supportElevation(MobileStructure unit, Coords coords) { |
Collaborator
There was a problem hiding this comment.
Any method that can return null as part of its WAI operations should have the @Nullable tag
Comment on lines
+95
to
+102
| for (IBuilding candidate : game.getBoard(boardId).getBuildingsVector()) { | ||
| if (!(candidate instanceof AbstractBuildingEntity building) || !BuildingConstruction.usesHexsides(building) | ||
| || building.isDestroyed()) { | ||
| continue; | ||
| } | ||
| building.getWallSegmentState().initialize(building); | ||
| for (CubeCoords hex : building.getInternalBuilding().getOriginalCoordsList()) { | ||
| for (int side = 0; side < 6; side++) { |
Collaborator
There was a problem hiding this comment.
Another loop within a loop within a loop
Comment on lines
+334
to
+339
| for (Entity entity : List.copyOf(getGame().getEntitiesVector())) { | ||
| if (!(entity instanceof MobileStructure mobile) | ||
| || !mobile.getNavalState().checkFootingThisRound(getGame().getRoundCount())) { continue; } | ||
| for (Coords coords : List.copyOf(mobile.getCoordsList())) { | ||
| int roof = BuildingElevation.roof(mobile, coords); | ||
| for (Entity rider : List.copyOf(getGame().getEntitiesVector(coords, mobile.getBoardId()))) { |
Collaborator
There was a problem hiding this comment.
loop within a loop within a loop
Member
Review: MegaMek PR #8946 "Buildings" (exeea/buildings -> main)Reviewed at head 6629ddc (merge-base = current main b3891f1). 283 files, +24,116 / -1,102. Checked against the project coding style guide and PR checklist. Verdict: NOT MERGEABLE as-is. Five blockers (B1-B5), then a long tail of should-fix items. The architecture is A. Blockers
B. Server-side gate
C. Monolithic-class gate
D. Deep dive: deployment facing (requested)Files:
E. Style and hygiene (0, 2)
F. Diagnostic logging
G. Performance
H. Tests
I. Automated review triageVerified each against the code. Accept the valid ones, decline the rest with the stated reason.
J. Spotless warning before you format
K. Public API surface
L. PR body and process
M. What is done well (keep)
|
Member
Follow-up: deployment orientation and duplicated logicTwo items I was asked to look at specifically after the main review. Deployment orientation (section D of the review, condensed)
Duplicated logic
Checked and not duplicated: the three collapse handlers compose rather than repeat; Overlap with an in-progress branch
It also names building hexes differently. That branch adds |
Member
Recommendation: resolving the duplicationFor each area: the one implementation that survives, what the others become, and the functionality each side needs kept. 1. Hex rotation: one method on the record
2. Facing predicates: two concepts, one home (
|
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
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.
This PR adds mobile buildings and the parts needed for buildings to be constructed in MegaMekLab