-
Notifications
You must be signed in to change notification settings - Fork 332
Unify MHQ Faction and FactionRecord and use yaml faction data files #6735
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
Conversation
Thank you so much for doing this, Juli. As someone who works with factions more than a little, I just wanted to say how appreciated this work is. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6735 +/- ##
============================================
+ Coverage 30.70% 30.74% +0.04%
- Complexity 17579 17596 +17
============================================
Files 2979 2986 +7
Lines 291298 291473 +175
Branches 50715 50731 +16
============================================
+ Hits 89436 89619 +183
+ Misses 195160 195149 -11
- Partials 6702 6705 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
if (!ratingLevels.isEmpty()) { | ||
pw.println("\t\t<ratingLevels>" + StringEscapeUtils.escapeXml10(String.join(",", ratingLevels)) | ||
+ "</ratingLevels>"); | ||
public void saveIfChanged() throws IOException { |
Check notice
Code scanning / CodeQL
Missing catch of NumberFormatException Note
return tags; | ||
} | ||
|
||
public List<FactionRecord.DateRange> getYearsActive() { |
Check notice
Code scanning / CodeQL
Exposing internal representation Note
after this call to getYearsActive
getYearsActive exposes the internal representation stored in field yearsActive. The value may be modified
after this call to getYearsActive
Are any of the faction codes in MHQ actually different? Because all of the planetary data depends on these.... |
The faction codes are generally equal aside from some MHQ factions that are missing in the RATs and vice versa and a very small number of, well, data bugs, e.g. we have three versions of the Tamar Pact with different codes. |
Taking a quick glance, the era mods might be an issue. Which mod works with which era? Needs labels. |
Don't suppose you could update the Bandit Caste (BAN code) to include the full range of equipment ratings? While under-equipped - top-rated Bandit Caste is certainly not the same as top-rated Clan forces - there should still be a range of availability from the Society-derived forces fielded by The Jaguar and Tanite/ex-Burrock forces to the dregs that are barely getting by. |
Whoops meant to quote reply and I edited above. If you are editing the factions, we should ensure these are not in use by planetary systems. |
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.
Pull Request Overview
This PR introduces individual YAML files under megamek/data/universe/commands
to represent each subfaction (command) with their metadata (key, name, active years, rating levels, fallback factions).
- Adds new YAML definitions for 20 commands, replacing the old XML-based setup.
- Aligns the fallback faction data retrieval with the new
Faction2
class mechanism.
Reviewed Changes
Copilot reviewed 372 out of 372 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
megamek/data/universe/commands/CC.TG.yml | Add Tikonov Guards command YAML |
megamek/data/universe/commands/CC.SSB.yml | Add Sarna Sabres Brigade command YAML |
megamek/data/universe/commands/CC.SIS.yml | Add St Ives Sentinels command YAML |
megamek/data/universe/commands/CC.SIJ.yml | Add St Ives Janissaries command YAML |
megamek/data/universe/commands/CC.SIAC.yml | Add St Ives Armored Cavalry YAML |
megamek/data/universe/commands/CC.SD.yml | Add Sian Lancers command YAML |
megamek/data/universe/commands/CC.MAC.yml | Add McCarron's Armored Cavalry YAML |
megamek/data/universe/commands/CC.LL.yml | Add Liao Lancers command YAML |
megamek/data/universe/commands/CC.LG.yml | Add Liao Guards command YAML |
megamek/data/universe/commands/CC.LCC.yml | Add Liao Cháng-Chéng command YAML |
megamek/data/universe/commands/CC.DC.yml | Add Death Commandos command YAML |
megamek/data/universe/commands/CC.CRC.yml | Add Confederation Reserve Cavalry |
megamek/data/universe/commands/CC.CR.yml | Add Chesterton Regulars command YAML |
megamek/data/universe/commands/CC.CHO.yml | Add Citizens' Honored command YAML |
megamek/data/universe/commands/CC.CHG.yml | Add Capellan Home Guard command YAML |
megamek/data/universe/commands/CC.CH.yml | Add Capellan Hussars command YAML |
megamek/data/universe/commands/CC.CDF.yml | Add Capellan Defense Force command |
megamek/data/universe/commands/CC.CC.yml | Add Capellan Chargers command YAML |
megamek/data/universe/commands/CC.CB.yml | Add Capellan Brigade command YAML |
megamek/data/universe/commands/CC.AH.yml | Add Andurien Hussars command YAML |
Comments suppressed due to low confidence (1)
megamek/data/universe/commands/CC.LCC.yml:2
- [nitpick] The name uses accented characters (
Cháng-Chéng
) while other entries use plain ASCII. Consider normalizing toLiao Chang-Cheng
for consistency.
name: Liao Cháng-Chéng
yearsActive: | ||
- start: 2367 | ||
end: 2823 | ||
- start: 3113 |
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.
The second yearsActive
entry has only a start date. If this period is meant to be open-ended, add a comment or document that convention; otherwise specify an end date for clarity.
- start: 3113 | |
- start: 3113 # Open-ended period, ongoing |
Copilot uses AI. Check for mistakes.
Hi @SJuliez You seem to be on a break but their is interest in merging this if its ready to go, can you advise? Cheers |
Well, there are conflicts so no merge at the moment. Also, I need to make some changes to factions for the big SMACK project (which I have not been able to find time for). I don't think that will likely affect this much as it is mostly about removing unused factions but its probably worth checking. |
# Conflicts: # megamek/src/megamek/client/ratgenerator/RATGenerator.java # megamek/src/megamek/client/ui/dialogs/randomArmy/ForceGeneratorOptionsView.java # megamek/src/megamek/client/ui/dialogs/randomArmy/RandomArmyDialog.java
Replaced by #7246 |
See the description and discussion in #6735, this PR replaces the old one and does not include the data; comes in separate PR. I wanted to have the data separate so it's easier to review
This PR together with MML and MHQ PRs aims to unify the faction data of MHQ and MM on the MM side to make it available everywhere and use individual yaml files instead of a single huge xml file.
This is pretty hefty for review, but when filtering out the .yml files, it's only a handful of classes. Ideally, the whole thing should have no visible effect in MM, MML or MHQ except fix a bug with MML faction selection which omitted a bunch of factions.