-
Notifications
You must be signed in to change notification settings - Fork 178
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
Updated & Fixed Required Combat Team Calculations; Reworked Combat Team Requirement Contract Pay Modifiers; Added Contract Difficulty Modifier #5664
Conversation
Replaced all instances of "RequiredLances" with "RequiredCombatTeams" to improve clarity and consistency with terminology. Updated related calculations, method names, variables, and documentation to reflect the new naming convention, ensuring compatibility across the codebase.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5664 +/- ##
============================================
+ Coverage 10.19% 10.20% +0.01%
- Complexity 6066 6076 +10
============================================
Files 1010 1010
Lines 139356 139316 -40
Branches 20450 20450
============================================
+ Hits 14207 14222 +15
+ Misses 123773 123720 -53
+ Partials 1376 1374 -2 ☔ View full report in Codecov by Sentry. |
Cleaned up a redundant HTML closing tag in the method documentation. This improves code clarity and adheres to proper formatting standards. No functional changes were made.
Code looks fine, though I'm not sure if some of the changes are good or even necessary, and at least doesn't do what you intended, if I'm not mistaken.
This has knock-on effects and means the "adjust contract payment for deployment limits" campaign option has no effect anymore. This is not a fix or an update, it's an actual rule change and should be discussed further before merging.
I can see the theoretical argument for not requiring a company-size (12 units) ComStar/WoB force to keep 2 ComTs deployed at all times, since unlike an IS-based company with three 4 unit Lances they'd have to keep their two 6 unit Level IIs in the field 24/7, but once again this has further effects and is effectively a rule change. Also, while you'd expect Level IIIs to be on par with battalions because they're the same size, as far as I can tell MHQ considers them to be a company equivalent.
Last but not least, this also has some unintended consequences. |
While true, users physically cannot assign more ComTs than the allowance determined by their campaign commander's strategy. So having contracts require more forces than this hard limit risks putting the player in a situation where they accept a contract and, without any warning, suddenly find themselves unable to actually perform the requirements of that contract. |
This is a good point. For some reason my brain was certain C* 'companies' were two Level IIs. |
Removed redundant "company-level formation size" calculations and replaced them with a fixed divisor for variance and bypass logic. Simplified the calculation logic in `calculateVarianceFactor` and `calculateBypassVarianceReduction` methods to improve clarity and maintainability. Adjusted auxiliary role check in `getEffectiveNumUnits` for consistency.
The hard limit of whatever the user has set up for maximum formations per Strategy is retained, because this is necessitated by the inclusion of that campaign option. Whether that campaign option should be removed is a discussion for another day and not within the scope of this PR. |
Yes, the AtB rules already prevent that. If you broke that by changing the rules and are now introducing a fix for it that changes the rules further and renders a campaign option inoperable, it should be split off and discussed instead of just plowing ahead.
That is an different campaign option and not what I was talking about.
I'm not entirely up-to-date, but I think roles are reset to Reserve upon accepting a contract, so there's still some suboptimal interactions here. |
Would you be able to clarify what campaign option you're referring to, I think that's a point of confusion. |
"Adjust contract payment for deployment limits" as checked by |
Sorry, the way you presented things made it seem like you were still discussing the cap on deployable ComTs based on Strategy. Or, at least, that's how I read it, leading to the confusion. |
I was not "still discussing the cap on deployable ComTs base on Strategy" because I never even started discussing that.
To clarify this: It was always possible for |
Closing pending rethink |
Sorry about the confusion, I really bungled the first explanation, and I have to apologize for attributing a bug to you that has actually been around for months. For what it's worth, I do completely agree with the goals. Capping the number of ComTs required to be deployed by the player at the maximum that can actually be deployed is entirely correct. As far as I can tell, and I've not had the time to go through all of the code, what is happening is that there's 3 different levels of required ComTs.
It is important that 1 and 2 can actually be different, because this affects the payment multiplier. Contracts with higher than average deployment requirements pay better than average. The payment adjustment for being unable to deploy as many ComTs as the employer would like, and the required number automatically rising to the new deployment cap as the strategy skill is increased (or settings are changed) were broken by commit Back to the actual PR: Hence my initial idea of flagging auxiliary forces in the TO&E like support forces and convoys. If we don't want to change or add rules we shouldn't make things too complicated like counting the a different percentage of equipment value for auxiliary vs main combat forces, but I'm still leaning towards counting for contract payment purposes. There is however an argument to be made for not counting auxiliary forces for payment because they already offer an advantage: Making it easier to win scenarios. |
I do think moving auxiliary to a special TO&E status, like Convoys, is absolutely the right move. I think the whole process of determining required ComTs is actually a bit of a problem. Because it's being touched upon by so many different things, resulting in hard to understand code. That's something we should take a look at tightening up, because being frank I know the mhq codebase better than most and even I got confused several times. I dread to imagine how someone would manage if they were just reviewing a PR or taking a cursory glance. That's just asking for trouble. |
Yeah, I do feel strongly about contract requirements being only based on the TO&E.
To be fair, the multiple values do have a use. But they should really use more distinct variable names and the transition to the abstract market has spread them out even further so it's become way too easy to think that something with "required" in the name is an actual requirement instead of a base value that'll still be modified up to twice.
Case in point. I thought you were going to break something, but it was already broken and no one noticed. |
# Conflicts: # MekHQ/src/mekhq/gui/dialog/NewAtBContractDialog.java
Simplified combat team utilization calculations to directly use proportion of required to total combat teams. Removed the unused 'adjustPaymentForStrategy' option and associated logic, streamlining campaign options and related UI elements.
Previously, the multiplier calculation did not account for cases where totalCombatTeams was zero, which could result in a division by zero error. Added a check to ensure the calculation only proceeds when totalCombatTeams is greater than zero. This improves the stability and reliability of the contract market logic.
Updated and Reopened PR
Fix #5713 |
Replaced hardcoded constants with COMBAT_FORCE_DIVIDER for clarity and consistency across methods. Enhanced pay multiplier logic by incorporating contract difficulty adjustments when FG3 is enabled. Simplified and standardized variance factor calculations for better maintainability.
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.
Reviewed with Co-Pilot and IDEA AI
Updated how we calculate the number of combat teams required by a contract.
Improved how we handle non-IS campaigns, so that we are no longer assume the campaign uses base-3 at the company-level. This will mainly benefit ComStar and WoB campaigns, as they're the only base-2 company-level factions I'm aware of.Fix #5713, Fix #5737