-
Notifications
You must be signed in to change notification settings - Fork 171
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
Added Contract Automation #5172
Conversation
Implemented automated prompts for contract initiation, including mothballing units, charting courses, and beginning transit. Added the ability to track and manage automatically mothballed units within the campaign.
Removed the "(GM)" designation from mothball and activation reports to streamline notification messages. Enhanced contract automation with automated unit activation upon entering the target system and improved dialogue prompts for transit operations.
Added "The MegaMek Team" copyright for 2024 to Unit.java, Contract.java, and ContractMarketDialog.java. This update ensures proper attribution and legal compliance for the project's source files.
Added functionality to handle automated mothballing of units in Campaign.java, including saving and loading from XML. Enhanced ContractAutomation dialog by refining employer name handling and transitioning to a JDialog. Verbose strings in properties files modified for better clarity.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5172 +/- ##
============================================
- Coverage 10.52% 10.51% -0.02%
+ Complexity 6042 6040 -2
============================================
Files 956 957 +1
Lines 134346 134513 +167
Branches 19514 19544 +30
============================================
- Hits 14145 14141 -4
- Misses 118851 119018 +167
- Partials 1350 1354 +4 ☔ View full report in Codecov by Sentry. |
I've always just run with x4 maint time and it hasn't been a problem during transit, buuuut I'm also using 'only break on A quality' and I don't know if I'm running with the default maintenence values. One thing I can't tell from glancing at the code, is does this mothball units currently under refit? Right now, I just checked, you can start a refit and then mothball a unit, and the refit will proceed. But you cannot refit a unit that is already mothballed. Does this need to change? |
My gut tells me we probably shouldn't allow refits on mothball units at all, and we should probably prevent the mothballing of units undergoing refit. |
Replaced the check for unit mothball status with a more comprehensive condition that includes availability and repair status. This ensures only appropriate units are added to the mothball targets list.
The outdated comments regarding SpecialAbility and error handling in processAutomatedMothballNodes were removed. This cleanup improves code readability and maintains the focus on current logic.
Updated to check unit is both available (is present, not deployed, not refitting, not mothballing, and not mothballed) and not under repairs. |
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.
Just did a quick code review.
I appreciate you. |
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.
Great idea this!
mothballedUnits.add(unit); | ||
} | ||
|
||
return mothballedUnits; |
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.
might simply return mothballTargets (?)
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.
Good point well made
* @param campaign The current campaign | ||
* @return The title of the commander, or a default string if no commander. | ||
*/ | ||
private static String getCommanderAddress(Campaign campaign) { |
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.
It feels like this can't be the first time this is done (or it might not be the last)... same for the speaker icon. But I wouldnt know. There is this "this contract is too difficult for your wimpy forces" dialog. Might be something to unify in the future.
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.
You're right, moved it to Campaign.java
mainPanel.add(descriptionPanel, BorderLayout.CENTER); | ||
|
||
// Create JOptionPane | ||
JOptionPane optionPane = new JOptionPane(mainPanel, |
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.
you are probably double-dialoging this because JOptionPane is not flexible, right?
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.
That was the intent, but on a second viewing I don't think it's necessary and double-dialoging isn't doing anything beneficial here, just unnecessarily increasing complexity.
Removed.
public static void performAutomatedActivation(Campaign campaign) { | ||
List<Unit> units = campaign.getAutomatedMothballUnits(); | ||
|
||
if (units.isEmpty()) { |
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.
it feels like this is unnecessary, the loop will simply skip if campaign.getAutomatedMothballUnits() is empty.
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.
This was a relic from an earlier iteration and should have been removed. Thanks for pointing it out.
activateUnitAction.execute(campaign, unit); | ||
MekHQ.triggerEvent(new UnitChangedEvent(unit)); | ||
} | ||
} |
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.
should this method clean up the automothball list? If getAutomatedMothballUnits() is not a copy and is modifiable:
units.removeIf(unit -> !unit.isMothballed());
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.
Thanks for mentioning this, that was an oversight
Extracted commander address logic into `Campaign` class. Enhanced UI components for contract automation dialogs and added detailed mothballing and activation reporting.
Updated to incorporate suggestions from Julie |
Currently accepting a contract is a bit of a process: you need to accept the contract, navigate to the Briefing Room, click on the system, click on Calculate Jump Path, click on Begin Transit. Then, by the time you arrive half of your forces are falling apart from failed maintenance checks.
To resolve that last problem, I'm aware that many of our players advise mothballing forces before transit. Seeing just how crippling RAW maintenance rules can be, I'm inclined to agree.
From a New Player Experience perspective, both of these issues present a couple of problems:
This PR attempts to resolve these options in a couple of steps: