Skip to content

feat: adds CONVOY behavior setting preset and flag to ignore damage output on pathing #6944

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

Merged
merged 5 commits into from
May 20, 2025

Conversation

Scoppio
Copy link
Collaborator

@Scoppio Scoppio commented Apr 28, 2025

What is it about?

Adds CONVOY behavior setting preset, a behavior controlling flag to ignore damage output (damage that would be caused by this unit onto enemies) when calculating pathing utility score was added, also adds special case where unit will give priority to face towards its destination when fleeing but only if it has the flag ignore damage output enabled.

What it does?

This small feature adds a simple behavior control toggle

Anything else?

Alot of Javadoc was added on BasicPathRanker to better explain what is happening.

Issues:

This PR potentially resolves the following RFE's

…king, also adds special case where unit will face destination
@Copilot Copilot AI review requested due to automatic review settings April 28, 2025 01:47
@Scoppio Scoppio requested a review from a team as a code owner April 28, 2025 01:47
@Scoppio Scoppio self-assigned this Apr 28, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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 adds a new CONVOY behavior preset along with a flag to ignore damage output, affecting both path ranking and behavior settings. Key changes include:

  • Introduction of new clamp methods (clamp01 and clampUlp1) in MathUtility.
  • Updates to BotConfigDialog and various bot components (UtilityPathRanker, BasicPathRanker, BehaviorSettings, etc.) to incorporate the ignoreDamageOutput flag.
  • Enhanced Javadoc documentation and minor logging improvements.

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
megamek/src/megamek/codeUtilities/MathUtility.java Added helper clamp methods with accompanying Javadoc
megamek/src/megamek/client/ui/dialogs/BotConfigDialog.java Added UI component and action handling for ignoreDamageOutput flag
megamek/src/megamek/client/bot/princess/UtilityPathRanker.java Updated path ranking logic to account for ignoreDamageOutput flag
megamek/src/megamek/client/bot/princess/Princess.java Logging improvements with parameterized messages
megamek/src/megamek/client/bot/princess/PathRanker.java Expanded Javadoc and minor logic updates for path ranking
megamek/src/megamek/client/bot/princess/BehaviorSettingsFactory.java Added new CONVOY preset and null-checks for behavior presets
megamek/src/megamek/client/bot/princess/BehaviorSettings.java Added new field and XML serialization for ignoreDamageOutput flag
megamek/src/megamek/client/bot/princess/BasicPathRanker.java Enhanced documentation and integrated ignoreDamageOutput flag in scoring
Files not reviewed (1)
  • megamek/i18n/megamek/client/messages.properties: Language not supported
Comments suppressed due to low confidence (1)

megamek/src/megamek/client/bot/princess/UtilityPathRanker.java:150

  • [nitpick] When the ignoreDamageOutput flag is enabled, both physicalDamage and firingDamage are set to 0. Please confirm that disabling both damage types is the intended behavior in all scenarios.
if (getOwner().getBehaviorSettings().isIgnoreDamageOutput()) {

@Scoppio Scoppio added Princess/AI Issues or PR that relate to the current Bot AI PACAR Any Issues related to the PACAR (Princess Abstract Combat Auto Resolve) System labels Apr 28, 2025
@Scoppio Scoppio marked this pull request as draft April 28, 2025 01:57
@Scoppio Scoppio added the For New Dev Cycle This PR should be merged at the beginning of a dev cycle label Apr 28, 2025
@Scoppio Scoppio marked this pull request as ready for review April 28, 2025 01:59
Copy link

codecov bot commented Apr 28, 2025

Codecov Report

Attention: Patch coverage is 29.22849% with 954 lines in your changes missing coverage. Please review.

Project coverage is 30.50%. Comparing base (d6c2db5) to head (ba6b1b9).
Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
...src/megamek/client/ui/dialogs/BotConfigDialog.java 0.00% 604 Missing ⚠️
.../megamek/client/bot/princess/BehaviorSettings.java 46.20% 210 Missing and 38 partials ⚠️
...k/client/bot/princess/BehaviorSettingsFactory.java 70.86% 67 Missing and 7 partials ⚠️
...megamek/client/bot/princess/UtilityPathRanker.java 0.00% 19 Missing ⚠️
...c/megamek/client/bot/princess/BasicPathRanker.java 14.28% 4 Missing and 2 partials ⚠️
megamek/src/megamek/codeUtilities/MathUtility.java 0.00% 2 Missing ⚠️
...amek/src/megamek/client/bot/princess/Princess.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6944      +/-   ##
============================================
+ Coverage     30.48%   30.50%   +0.02%     
- Complexity    16321    16431     +110     
============================================
  Files          2896     2934      +38     
  Lines        284892   285202     +310     
  Branches      49588    49594       +6     
============================================
+ Hits          86838    87013     +175     
- Misses       191971   192105     +134     
- Partials       6083     6084       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SuperStucco
Copy link
Collaborator

The clamp01() method indicates "The value if it is inside the range given by the limits (inclusive)", but it only has one input - the value to be clamped. It should include either values to clamp between as arguments, or the JavaDoc updated to show that it is clamping between 0.0 and 1.0 (or whatever values are hard coded).

Copy link
Collaborator

@rjhancock rjhancock left a comment

Choose a reason for hiding this comment

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

Im glad to see more documentation get added in.

That being said, the nitpick CoPilot picked out is valid. :)

@Scoppio
Copy link
Collaborator Author

Scoppio commented May 12, 2025

The clamp01() method indicates "The value if it is inside the range given by the limits (inclusive)", but it only has one input - the value to be clamped. It should include either values to clamp between as arguments, or the JavaDoc updated to show that it is clamping between 0.0 and 1.0 (or whatever values are hard coded).

Well... it is the CLAMP 0 1 method....

@Scoppio Scoppio added the Conflicts The PR has merge conflicts that need to be resolved label May 12, 2025
@rjhancock rjhancock removed the For New Dev Cycle This PR should be merged at the beginning of a dev cycle label May 20, 2025
@Scoppio Scoppio requested a review from a team as a code owner May 20, 2025 05:07
@Scoppio Scoppio force-pushed the feat/convoy-ai branch 2 times, most recently from aeaace0 to aab0d15 Compare May 20, 2025 05:12

Transformer transformer = TransformerFactory.newInstance().newTransformer();
transformer.setOutputProperty(OutputKeys.INDENT, "yes");
transformer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", "2");

Check warning

Code scanning / CodeQL

Potential output resource leak Warning

This FileWriter is not always closed on method exit.
@Scoppio Scoppio merged commit 3cb57a3 into MegaMek:master May 20, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Conflicts The PR has merge conflicts that need to be resolved PACAR Any Issues related to the PACAR (Princess Abstract Combat Auto Resolve) System Princess/AI Issues or PR that relate to the current Bot AI
Projects
None yet
3 participants