MegaMek / megamek

MegaMek is a networked Java clone of BattleTech, a turn-based sci-fi boardgame for 2+ players. Fight using giant robots, tanks, and/or infantry on a hex-based map.
http://www.megamek.org
GNU General Public License v2.0
295 stars 282 forks source link

Role typo in forcegenerator parse code #5267

Open SuperStucco opened 5 months ago

SuperStucco commented 5 months ago

Should be an easy one for newcomers...

In the forcegenerator XMLs there is a role element, that includes the option for designating infantry as Anti-Mech equipped/specialized/whatever - 'anti_mek'. In MissionRole.java the processor uses two alternatives:

https://github.com/MegaMek/megamek/blob/5808376bf3f0f0e3f2ebc383ea9b73f4e6b60660/megamek/src/megamek/client/ratgenerator/MissionRole.java#L706-L708

Note that it accepts a space or a dash, but not an underline. As the underline version is already used in the forcegenerator XML files (and I have continued this with my own personal customizations), I would recommend replacing the two unused entries with the version in use. This will also match the text in the docs/RAT Stuff/rat-generator.txt help file.

Edit: jumped the gun a bit, looks like there are multiple instances of this. Here is a quick summary, based on the XMLs and the rat-generator.txt help file:

SuperStucco commented 5 months ago

A quick update - I know there's some processing going on in the switch statement with

'role.toLowerCase().replace("_", " ")'

but having the data, the help files, and the code all consistently saying the same thing is a huge help when trying to diagnose problems as well as doing data entry. I was trying to track down some issues for infantry field gun platoons and so went looking for the role "field_gun" as it is in the data files... nothing found because the case statement is trying to match the string "field guns". End result when code is run is the same but massaging things this way ends up creating other issues and potentially masking problems.

IllianiCBT commented 5 months ago

I would recommend replacing the two unused entries with the version in use.

By this do you mean to remove the unused cases and replace with underscore, or to add new cases to account for the missing underscore cases in the parser?

Once you reply, I should be able to knock this out very quickly.

SuperStucco commented 5 months ago

I would recommend replacing the two unused entries with the version in use.

By this do you mean to remove the unused cases and replace with underscore, or to add new cases to account for the missing underscore cases in the parser?

Once you reply, I should be able to knock this out very quickly.

Replace both instances with the only one in use in the data file i.e.

case "anti_mek:"

While you're working on these please review both the rat-generator.txt help file as well as the contents of the .../data/forcegenerator/[year].XML files to see what is actually in use (these can be found inside <role> elements, so that data == code == help file. If you do that, the text processing embedded in the switch statement can be removed as well.

If you're feeling generous, it might be an idea to throw in a bit of error logging in the default block at the bottom of the function, so that if a non-recognized role is present it can be found in the log, searched for in the XML, and fixed rather than only silently returning null.

IllianiCBT commented 5 months ago

After investigating this, I've found that other functions are relying on the parser remaining as currently implemented. I looked around for a couple of hours, but was unable to unknot the interactions.

Therefore, I stopped at cleaning up MissionRole.java and rat-generator.txt. Please see #5289 for more details.

SuperStucco commented 5 months ago

Can you link to a few of those functions? I'd like to do some checking.

IllianiCBT commented 5 months ago

Can you link to a few of those functions? I'd like to do some checking.

That's the problem. I could only find where the error reports were being pushed to megamek.log.

It's possible that it's generating false positives, but at after a couple hours fiddling about I concluded it was above my skill level to resolve. So, figured I'd do the best I could with the tools available.