-
Notifications
You must be signed in to change notification settings - Fork 339
PR MegaMek/mekhq#8544: Implement BV Calculation for TO:AR Advanced Buildings and Replace Generic BV #7861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…RAT and Experimental Rule to enable them in campaigns
…RAT and Experimental Rule to enable them in campaigns
…RAT and Experimental Rule to enable them in campaigns
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7861 +/- ##
============================================
- Coverage 29.81% 29.72% -0.09%
- Complexity 16235 16263 +28
============================================
Files 3125 3130 +5
Lines 300415 301766 +1351
Branches 52538 52836 +298
============================================
+ Hits 89569 89714 +145
- Misses 201616 202777 +1161
- Partials 9230 9275 +45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements Battle Value (BV) calculation for TO:AR Advanced Buildings by creating a new AbstractBuildingEntityBVCalculator class and updating BuildingEntity to return a non-zero generic BV. The primary motivation is to fix an infinite loop that occurred when generic BV was 0 during scenario creation.
Key Changes:
- New AbstractBuildingEntityBVCalculator implementing TO:AUE rules for structure BV calculation (Defensive BV = (Armor × 2.5 + CF × 1.5) × 0.5; Offensive BV = (Weapon BV + Hex Count × 50) × Speed Factor)
- BuildingEntity.getGenericBattleValue() now returns calculateBattleValue() instead of 0
- BVCalculator factory updated to route AbstractBuildingEntity instances to the new calculator
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| AbstractBuildingEntityBVCalculator.java | New BV calculator implementing TO:AUE structure-specific rules with custom armor, structure, and weight processing |
| BuildingEntity.java | Changed getGenericBattleValue() to return calculated BV instead of 0 to prevent infinite loops |
| BVCalculator.java | Added instanceof check to route AbstractBuildingEntity to new calculator before fallback to CombatVehicleBVCalculator |
| BuildingEntityTest.java | Updated test expectation to match new behavior where generic BV equals calculated BV |
Critical Issues Found in New AbstractBuildingEntityBVCalculator (Cannot comment directly on new files):
The new AbstractBuildingEntityBVCalculator has two critical bugs that will cause incorrect BV calculations for multi-hex buildings:
-
Line 68:
processArmor()usesentity.getOArmor(0)which only retrieves armor from location 0. Buildings can span multiple hexes with different armor values per location. This will significantly undercount total armor for multi-hex buildings. Should useentity.getTotalOArmor()instead to sum armor across all locations. -
Lines 82-84:
processStructure()retrieves CF from only the first hex:building.getCoordsList().get(0)and thenbuilding.getCurrentCF(firstHex). For multi-hex buildings, this only accounts for one hex's construction factor. Should useentity.getTotalOInternal()to sum CF across all locations.
These bugs mean that any building spanning more than one hex will have drastically undercounted defensive BV, making them appear much weaker than they should be for game balance purposes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The "critical issue" here is incorrect. The CF & Armor value are not scaled per hex, the hex scaling is handled elsewhere. |
Sleet01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Support for Advanced Building Gun Emplacement RAT and Experimental Rule to enable them in campaigns. Required for MegaMek/mekhq#8544 to function properly. A Generic BV of 0 causes an infinite loop when creating a scenario. I don't really know how it was calculated for other units, so for now I'm just returning the actual BV.