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

feat: slightly better serializer for unit actions and state, added attack action too #6834

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Scoppio
Copy link
Collaborator

@Scoppio Scoppio commented Apr 6, 2025

What it does?

Improves the current TSV Serializer and the actions for the game dataset logger.

  • Adds more fields too for UnitAction and UnitState
  • Adds AttackUnitAction
  • and a few new function for EntityFeatureUtils that can gets the percent of armor on each "side" of a unit. Mostly used exclusively for Dataset logger right now, but will be used for other things on the feature extractor/axis calculator for caspar at a later date.
  • Cover all data that was missing for custom units, and the info that was missing and I had to extract directly from MegaMek to run my model trainer.

@Scoppio Scoppio self-assigned this Apr 6, 2025
@Scoppio Scoppio requested review from Sleet01 and HoneySkull April 6, 2025 04:03
@Scoppio Scoppio force-pushed the feat/dataset-loger-improvements branch from 3f2468e to b8e250b Compare April 6, 2025 04:03
@Scoppio Scoppio force-pushed the feat/dataset-loger-improvements branch from b8e250b to 3cbd32b Compare April 6, 2025 04:06
Copy link

codecov bot commented Apr 6, 2025

Codecov Report

Attention: Patch coverage is 41.47399% with 405 lines in your changes missing coverage. Please review.

Project coverage is 30.12%. Comparing base (95a1af5) to head (116302e).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
megamek/src/megamek/ai/dataset/UnitAttack.java 25.71% 78 Missing ⚠️
megamek/src/megamek/ai/dataset/UnitAction.java 35.92% 66 Missing ⚠️
megamek/src/megamek/ai/dataset/UnitState.java 38.46% 56 Missing ⚠️
megamek/src/megamek/ai/dataset/GameData.java 15.90% 37 Missing ⚠️
...k/src/megamek/ai/dataset/EntityDataSerializer.java 28.57% 35 Missing ⚠️
megamek/src/megamek/common/GameDatasetLogger.java 19.35% 25 Missing ⚠️
...ek/src/megamek/ai/dataset/BoardDataSerializer.java 8.00% 23 Missing ⚠️
megamek/src/megamek/ai/dataset/BoardData.java 19.23% 21 Missing ⚠️
...mek/src/megamek/ai/dataset/GameDataSerializer.java 12.50% 21 Missing ⚠️
megamek/src/megamek/ai/dataset/EntityDataMap.java 0.00% 17 Missing ⚠️
... and 4 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6834      +/-   ##
============================================
+ Coverage     30.05%   30.12%   +0.07%     
- Complexity    15587    15621      +34     
============================================
  Files          2875     2884       +9     
  Lines        282748   283107     +359     
  Branches      49302    49296       -6     
============================================
+ Hits          84988    85299     +311     
- Misses       192210   192246      +36     
- Partials       5550     5562      +12     

☔ 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.

@Scoppio Scoppio force-pushed the feat/dataset-loger-improvements branch 3 times, most recently from fd7fbb0 to 44d6fd4 Compare April 6, 2025 20:32
@Scoppio Scoppio force-pushed the feat/dataset-loger-improvements branch from 44d6fd4 to 2f6f202 Compare April 6, 2025 20:33
@Scoppio Scoppio requested a review from Copilot April 6, 2025 21:48
Copy link

@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.

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

@Scoppio Scoppio requested a review from Copilot April 6, 2025 21:54
Copy link

@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.

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

@Scoppio Scoppio requested a review from Copilot April 6, 2025 22:22
Copy link

@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.

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

megamek/src/megamek/common/Compute.java:474

  • Verify the removal of the 'false' parameter from the climb method call to ensure that the change in behavior is intentional.
game.getBoard().getHex(coords), elevation, climbMode);

megamek/src/megamek/ai/dataset/UnitAction.java:62

  • The update of the initial value in the reduce operation from 0.0 to 1.0 is crucial for correctly computing the chanceOfFailure; please document or verify that multiplying the PSR values was the intended fix.
double chanceOfFailure = SharedUtility.getPSRList(movePath).stream()
                                       .map(psr -> psr.getValue() / 36d)
                                       .reduce(1.0, (a, b) -> a * b);

megamek/src/megamek/ai/dataset/UnitStateSerializer.java:70

  • Ensure that additional fields (like HAS_ECM and armor side percentages) introduced in the UnitState record are adequately tested in the serializer tests.
row[UnitStateField.HAS_ECM.ordinal()] = obj.hasEcm() ? "1" : "0";

@Scoppio Scoppio requested a review from rjhancock April 7, 2025 20:48
@Scoppio Scoppio force-pushed the feat/dataset-loger-improvements branch from cfc9e35 to d0df602 Compare April 7, 2025 21:50
@Scoppio Scoppio requested a review from rjhancock April 7, 2025 21:53
Copy link
Collaborator

@Sleet01 Sleet01 left a comment

Choose a reason for hiding this comment

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

After some discussion on Discord I have changed my mind on the records approach.
Everything looks good, excited to see the next steps!

@Scoppio Scoppio force-pushed the feat/dataset-loger-improvements branch from 431ab09 to ca4c558 Compare April 10, 2025 23:41
@Scoppio Scoppio marked this pull request as draft April 10, 2025 23:47
@Scoppio
Copy link
Collaborator Author

Scoppio commented Apr 10, 2025

After some discussion on Discord I have changed my mind on the records approach. Everything looks good, excited to see the next steps!

And I changed my mind... this is really annoying to improve upon, so I am throwing the towel and going for the thing I didnt wanted to do before but is possibly the correct solution, using an EnumHashMap to hold the values and use reflection to serialize the object.

Copy link
Collaborator

@Sleet01 Sleet01 left a comment

Choose a reason for hiding this comment

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

Looks good! Nice job compacting some of that code down.

@Scoppio Scoppio marked this pull request as ready for review April 12, 2025 04:47
@Scoppio Scoppio requested a review from Copilot April 12, 2025 04:47
Copy link

@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.

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

Comments suppressed due to low confidence (1)

megamek/src/megamek/common/Compute.java:474

  • The removal of the false boolean argument in the climb method call appears intentional. Please verify that the change aligns with the updated method signature and intended behavior.
elevation, climbMode);

Comment on lines +208 to 209
logger.error(ex, "Error logging Attack unit action");
}
Copy link
Preview

Copilot AI Apr 12, 2025

Choose a reason for hiding this comment

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

The logger.error call here reverses the parameter order compared to other error logs. For consistency and clarity, update it to logger.error("Error logging Attack unit action", ex).

Suggested change
logger.error(ex, "Error logging Attack unit action");
}
logger.error("Error logging Attack unit action", ex);

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The original code is correct

@Scoppio Scoppio force-pushed the feat/dataset-loger-improvements branch from 7b195f9 to 116302e Compare April 12, 2025 04:51
@Scoppio Scoppio dismissed rjhancock’s stale review April 12, 2025 04:55

already fixed

* @param field The field enum
* @return The string representation for a null value
*/
protected String getNullValue(F field) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'field' is never used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants