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

Begin work on implementing CamOps Contract Market #4644

Merged
merged 49 commits into from
Sep 25, 2024

Conversation

Algebro7
Copy link
Collaborator

@Algebro7 Algebro7 commented Aug 15, 2024

This starts work on refactoring the Contract Market system to support different rulesets such as CamOps. The code was extremely tightly coupled with the AtBContractMarket type and needed to be refactored to properly support other market types. I followed Windchild's suggestions throughout the codebase and extracted much of the logic into an AbstractContractMarket class which AtBContractMarket, CamOpsContractMarket, and DisabledContractMarket all inherit from.

Additionally, several areas in AtBContract were tightly coupled with AtBContractMarket, so I've extracted much of that logic into methods in the abstract market class. I think the code is much easier to read now and implementing a new market method later should be a lot easier, although there is still work to do.

Any feedback is welcome while I work on this. Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.45%. Comparing base (5173607) to head (dcf55ed).
Report is 144 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4644      +/-   ##
============================================
+ Coverage     10.29%   10.45%   +0.15%     
- Complexity     5866     5978     +112     
============================================
  Files           945      953       +8     
  Lines        131468   132333     +865     
  Branches      19073    19206     +133     
============================================
+ Hits          13533    13833     +300     
- Misses       116618   117172     +554     
- Partials       1317     1328      +11     

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

MHQXMLUtility.writeSimpleXMLCloseTag(pw, --indent, "contractMarket");
}

public static AbstractContractMarket generateInstanceFromXML(Node wn, Campaign c, Version version) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a lot of single or two letter variable names in this method, I'm assuming it was taken from another part of the codebase and then modified to meet your needs, but it makes for really hard to read code. Especially when there are a lot of different variables to track.

@Algebro7 Algebro7 force-pushed the refactor-contract-market branch from c460368 to 1c5f446 Compare August 16, 2024 00:42
@Algebro7 Algebro7 force-pushed the refactor-contract-market branch from 20110db to 1c03c7d Compare August 25, 2024 18:57
@Algebro7 Algebro7 force-pushed the refactor-contract-market branch 2 times, most recently from f22d754 to f222e11 Compare September 15, 2024 02:31
@Algebro7 Algebro7 force-pushed the refactor-contract-market branch from f222e11 to 0c7b255 Compare September 18, 2024 18:41
@@ -2246,6 +2240,18 @@ public Person findBestInRole(PersonnelRole role, String skill) {
return findBestInRole(role, skill, null);
}

public @Nullable Person findBestAtSkill(String skill) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a heads up, we're moving towards having everything JavaDoc'd, obviously not necessary while this is a WIP but something you'll likely need to address before it gets merged. We're aware the majority of our stuff isn't currently JavaDoc'd, so that's something we're trying to address. So if you create a new Class, Method, or edit an existing Class or Method please add JavaDoc's*

*This is going to be added to the automated tests at some point

}
}

private class ContractModifiers {

Check notice

Code scanning / CodeQL

Inner class could be static Note

ContractModifiers should be made static, since the enclosing instance is not used.
@Algebro7 Algebro7 force-pushed the refactor-contract-market branch from cb38917 to 1d4f06f Compare September 19, 2024 01:24
/**
* @return a list of currently active contracts on the market
*/
public List<Contract> getContracts() {

Check notice

Code scanning / CodeQL

Exposing internal representation Note

getContracts exposes the internal representation stored in field contracts. The value may be modified
after this call to getContracts
.
@Algebro7
Copy link
Collaborator Author

Ok, I'm marking this ready for review. There should be no changes in behavior for the existing AtBMonthly market type, but Camops is now selectable in the options and playable, although it still uses AtB for many of the systems until I can fully implement those. I've added a note to the Market Option dropdown and tooltip saying that the CamOps market is in development and users should not expect it to work properly yet.

Future work will revolve around overriding AbstractContractMarket methods, which mostly follow the AtB system, to instead follow CamOps. I've set this up in a way where it should be able to be done piecemeal instead of all at once.

@Algebro7 Algebro7 marked this pull request as ready for review September 19, 2024 19:08
return findMissionType(getReputationModifier(campaign), employer.isISMajorOrSuperPower());
}

private void setContractClauses(AtBContract contract, Campaign campaign) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'contract' is never used.
return findMissionType(getReputationModifier(campaign), employer.isISMajorOrSuperPower());
}

private void setContractClauses(AtBContract contract, Campaign campaign) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'campaign' is never used.
@Gribbly1
Copy link

If this is going to be a close implementation of CO contracts I would recommend some toggle options for fine tuning, mainly to avoid some edge cases:

  • turn reputation factor payment multiplier off
  • turn mission negotiation MoS adjustment for mission table off
  • do not include ammunition in peacetime costs
  • do not include fuel in peacetime costs

Employer categories can cause issues in some eras, depending upon which nations exist.

The combination of straight support and battle compensation in CO is a straight downgrade, rolling high can be a massive disadvantage with 'full fat' peacetime costs.

@Algebro7
Copy link
Collaborator Author

@Gribbly1 those are great suggestions, thanks! Would you mind putting them in an RFE? We intend to add configuration options as much as possible but I think a lot of people don't have direct experience with CamOps RAW, so it's hard to guess where the rough edges are sometimes.

@IllianiCBT
Copy link
Collaborator

With the size of this PR I'm wanting to get it merged to avoid any major conflicts that might come up while I'm issue report busting. Is it in a stage where it's feature complete enough to be merged, even if feature incomplete, or does it still need time in the oven?

@Algebro7
Copy link
Collaborator Author

It's ready to be merged--the player can use this contract market normally, even though the contract parameters won't be 100% correct yet. Nothing should have changed behavior-wise for the current AtB method but I would like to have feedback from the nightly testers to confirm that.

Thanks!

@IllianiCBT
Copy link
Collaborator

Cool, I'll get dinner out of the way and then do a review. Will get it merged ASAP.

@HammerGS
Copy link
Member

This has picked up conflicts

# Conflicts:
#	MekHQ/src/mekhq/campaign/Campaign.java
#	MekHQ/src/mekhq/campaign/market/AtbMonthlyContractMarket.java
@Algebro7 Algebro7 force-pushed the refactor-contract-market branch from 3c15419 to dcf55ed Compare September 25, 2024 22:56
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.

I'm happy with everything here, but I do think we just need to get this merged so you're not having to deal with conflicts. My stuff is a lot easier to fix than yours, by merit of size.

I have some requests moving forward:

  • The ability for 'Mercenary' to be the enemy faction
  • Consideration for Kurita's 'Death to Mercenaries' edict, so mercenaries stop getting contracts from the Combine during that period.

You might have implemented some of this, but I didn't spot it while I was reviewing.

@IllianiCBT IllianiCBT merged commit ab4b419 into MegaMek:master Sep 25, 2024
4 checks passed
@Algebro7
Copy link
Collaborator Author

Those are good ideas. #4878 also started a list of features someone with a lot of CamOps experience suggested. I'm planning to keep track of those in a "meta" issue so we can tick them off as they're implemented.

@Algebro7 Algebro7 deleted the refactor-contract-market branch September 26, 2024 00:20
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.

5 participants