Skip to content
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

Fix AtBMonthlyContractMarket payment multiplier #4954

Merged

Conversation

Algebro7
Copy link
Collaborator

There was an error when calculating the payment multiplier using the CamOps unit rating method for AtBMonthly contracts, which was caused by incorrectly using the reputation score instead of the reputation modifier for determining this multiplier. This PR does a couple of things:

  1. Fixes getReputationFactor() to use the camopsUnitRatingMod instead of the reputation score
  2. Reverts the change in AtBMonthlyContractMarket and uses the AtBUnitRatingMod instead
  3. Adds a getCamOpsUnitRatingMod() method to get the camops unit rating mod and support getReputationFactor()
  4. Adds docstrings for everything

Closes #4952

*
* @return The unit rating modifier as described in Campaign Operations.
*/
public int getCamOpsUnitRatingMod() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there value in moving this into the Reputation class - cooking dinner so the class might be called something else? Call the new method something like getReputationModifier().

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did actually try to do that first, but there is already a different getReputationModifier() there that does something based on the unit experience levels instead. I wasn't sure how that was used and so I didn't want to mess with it--could you look at that when you get a minute and let me know what you think?

Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll take a look once I’ve had dinner.

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.45%. Comparing base (980d504) to head (b3341a5).
Report is 9 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4954      +/-   ##
============================================
+ Coverage     10.44%   10.45%   +0.01%     
- Complexity     6005     6016      +11     
============================================
  Files           951      951              
  Lines        132875   132876       +1     
  Branches      19329    19329              
============================================
+ Hits          13876    13894      +18     
+ Misses       117648   117632      -16     
+ Partials       1351     1350       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@IllianiCBT IllianiCBT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Algebro7
Copy link
Collaborator Author

Algebro7 commented Oct 1, 2024

Thanks for the review, this should be fixed. I'm working on tests but that will likely take some more time/refactoring so I'll push those in a separate PR later.

@IllianiCBT IllianiCBT merged commit 9da71cd into MegaMek:master Oct 2, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[0.50.01-SNAPSHOT] AtB contract market generates contracts with very high base pay values.
3 participants