More detailed auto-selection of bombs #4164

SuperStucco · 2024-06-06T22:00:32Z

Randomly generated units that are cleared to carry bombs currently do so in a somewhat blunt manner - pick a random bomb type, load up to maximum based on carrying capacity until it's just out of thrust. This PR adds more randomness to the selection process, puts in a few guards to maintain a minimum thrust level, and produces more role-focused selections. It's a bit chunkier than I would like but should be a good start.

The new process looks like this:

Randomly determine how many of the fighters will carry external ordnance, somewhere around half to all
For each fighter, calculate a maximum load using minimum thrust values as well as carrying capacity. Some additional randomness is included.
Randomly select a loadout such as "Normal", "Anti-Mech", or "Anti-Conventional". Each loadout is a list of appropriate ordnance types with proportional values. Pirates, including "Unidentified Hostiles" get their own selections of low tech/high chaos loadouts.
Bombers deployed for space or low altitude combat will only get air-to-air loadouts (no HE, cluster, inferno, etc.), while those for ground support will mostly be air-to-ground loadouts. A few will be configured as CAP/combat air patrol rather than ground support.
Randomly generate ordnance using the proportions in the selected loadout, up to the calculated limit. Ordnance unavailable due to era restrictions is replaced with either HE bombs (ground only) or rocket launchers (ground and air). Lower rated forces (typically Dragoons rating D/F) will randomly have some advanced ordnance randomly replaced in the same way.
If any guided ordnance is selected (laser/TAG guided bombs, homing Arrow IV) some of the remaining fighters without ordnance will get an external TAG

Differences from the current system:

overall lighter bomb loads (but the heavies will still hurt)
fewer single-type loads, and more mixed loads e.g. HE and rockets, or cluster and inferno
large ordnance like Arrow IV, anti-ship, ASEW will only be common with high (A/B) rated forces
slight increase in HE bombs and rockets over other bomb types in general, particularly in earlier eras and lower rated forces

Should help resolve #2674

Eventually, I think this should switch to reading values from an external data file rather than using hard-coded values, and probably be pulled into it's own class as well to clean up the current location. That can wait until after the general process/framework is validated.

…te method previously used.

…modate (undocumented!) input requirements for setting bombs on Entity.

IllianiCBT · 2024-06-06T22:09:53Z

How will this interact with @Sleet01 ’s special munitions distribution system?

Sleet01 · 2024-06-06T22:27:06Z

How will this interact with @Sleet01 ’s special munitions distribution system?

@SuperStucco @IllianiCBT I think we can separate the two initially, then integrate them as necessary over the next release or two (after 0.49.20):

I am pulling the existing AtB bomb randomizer code out of MML so it will be available in MM games, which means we can have this new code live in AtB for now and they shouldn't interfere with each other.
I don't have this level of role definition so SuperStucco's work will likely be better for MHQ games, providing more immersion.
My updates are built around the new file format that users can modify and use to define their own loadouts for chassis, model, or even specific unit, which could be integrated into this new randomizer / generator code fairly easily - although there is also an argument to be made for adding a role and weight class component to the file format, similar to this setup.

@SuperStucco I would say that moving code that you want to extract into a new class is better done sooner than later, so that you can set up unit tests early and validate basic functionality more easily.

I'll do a full code review when I'm back on broadband, but based on a quick scan I don't think there's any reason our work couldn't be integrated into one seamless system for outfitting units.

Sleet01 · 2024-06-06T22:38:04Z

@SuperStucco There's a thread on the MegaMek Discord server where we discussed some of the stuff I was doing in my work: https://discord.com/channels/458705327911731231/1222035892965736539

IllianiCBT · 2024-06-06T22:48:53Z

I'm pleased to hear that, because I'm greedy and want both :D

SuperStucco · 2024-06-06T23:27:12Z

@SuperStucco I would say that moving code that you want to extract into a new class is better done sooner than later, so that you can set up unit tests early and validate basic functionality more easily.

I'm taking a more 'eating the elephant' approach with more small bites, as I get up to speed. While this gets reviewed I'm looking at a couple of other things - having field gun infantry generation respect the availability values, and potentially generating marine/XCT infantry in place of standard infantry in hostile environment conditions.

IllianiCBT · 2024-06-07T00:40:44Z

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java

+            if (numBombers >= maxBombers) {
+                break;
+            }
+


Is it worth adding a check here? I'm guessing that if the unit is unarmed it's not going to need any bombs.

if (isUnarmed) { continue }

Possibly relevant: The Boeing Jump Bomber is specifically designed to drop bombs, but has no internal weapons.

Carried this logic forward from the previous test. If it's carrying bombs, it really should be loaded up as that's the only realistic contribution it can make. Lower down in the code, they are specifically excluded from counting towards the total number of loaded bombers.

If anything they would be prioritized over armed units. Would it be worth re-ordering the bomber subset List<> to put unarmed entities first so they always get loaded up?

No, if that's the case then it's likely fine how it is. I hadn't spotted the previous test where it was defined :)

IllianiCBT · 2024-06-07T00:48:33Z

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java

+                randomThreshold = 80;
+                break;
+            default:
+                randomThreshold = 99;


I'm wondering whether we should throw an exception here, instead of assigning a default value to randomThreshold. There shouldn't be any point in which quality isn't within the F-A* bounds, so having an exception thrown would mean we get alerted to the problem rather than it failing silently.

Currently the method is called correctly, but the quality parameter is a straight int type. So it could be called inappropriately with bad values (negative, large numbers, etc.).

So, using a
throw new IllegalArgumentException("Unrecognized rating value: " + quality);
statement, and let the thrown exception get handled upstream?

Yeah, my thinking is that if we throw an exception we know something has gone wrong; if we silently handle it with a default value the only way we know there was a bad call is if someone happens to notice an unusually large quantity of rockets being handed out.

IllianiCBT · 2024-06-07T00:52:09Z

I was curious in seeing how everything worked, so I had a scroll through your code. I love the use of Map here, that's something I've only started using and it's great seeing just how powerful maps can be.

I did have a couple of points of feedback, nothing major, just thoughts that plonked into my noggin while I was reading. If Sleet comes along and disputes anything I've commented, assume they're right and I'm wrong. :D

HammerGS · 2024-06-08T19:36:14Z

Should this be considered a draft, or is it ready to go.

Sleet01 · 2024-06-09T01:44:01Z

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java

+    /**
+     * Distribution of various external ordnance choices for non-pirate forces
+     */
+    private static final Map<String, Integer> bombMapGroundSpread = Map.ofEntries (


It would be useful to know what these numbers represent: relative weights, percentages, absolute counts?

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java

Sleet01 · 2024-06-09T02:21:45Z

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java

                }

-                if (numBombers >= maxBombers) {
-                    break;
+                ((IBomber) curBomber).setBombChoices(generatedBombs);


I don't like this construction:

We're mixing bomb configuration and bomb setting; this should be separate for cleanliness and ease of parsing.

We duplicate this call in two places, but ~~neither will get called if hasGuided is false so if there are no guided munitions, no bombs will get set on any Aero unit.~~ actually, I see that setBombChoices() is outside the top test on hasGuided, but this illustrates my point re: parsing.

We have a hard-coded set of guided munitions ~~that doesn't include Aero Guided Arrow IV Bomb munitions,~~ (B_HOMING is Aero Homing AIV bomb) ; this should probably be a set or list that we declare up with the other constants.

~~It looks like the second option erases all bombs for the current unit and just gives it one single TAG pod, which I guarantee is not what we want.~~ Upon re-reading the code, I see that this is the else for the outer conditional, and only applies to ASFs without bombs. However, this seems to countermand the comments above stating that we are selecting a subset of ASF units to be bombers. I'm not sure I see a reason to track the number of bombers when we may add TAG to all non-selected bomber candidates anyhow.

I would actually suggest splitting the TAG-adding code out into a separate helper that just mutates a generatedBombs array based on the current bomb load on the bomber, the bomber's weight, and perhaps the bomber's loaded speed - that way we can avoid pushing a bomber from "fast enough" to "too slow" by adding the TAG pods only to units that have a slot to spare before dropping down one thrust rating.

The TAG part might be cleaned up a bit. The reasoning on that is, you can't use a TAG pod on the same round you're dropping bombs. So can't self-designate for laser/TAG guided bombs, and while you can self-designate for a homing Arrow that you dropped last round, the minimum movement of fixed wing aircraft means the chances they can be over the target zone the very next round is slim. So TAG designators get pushed to any fixed wing aircraft that did NOT get any external ordnance (*edit - or units which have ordnance but nothing guided), which happens from the random limit on how many units to load ordnance on to. That means units with external TAG will only have the one unit and will always have sufficient thrust; it also means that if all units are loaded with ordnance any guided ordnance will need ground designators or built-in TAG.

Sleet01 · 2024-06-09T02:30:04Z

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java

+                    (generatedBombs[BombType.B_LG] > 0 ||
+                            generatedBombs[BombType.B_HOMING] > 0);


See comment below; instead of two separate locations with different tests of hard-coded munitions, we should have a set or list of guided munitions that we can use to check for matches, ~~and it should include ASF Homing Arrow IV bomb munitions as well~~ (BombType.B_HOMING is ASF Homing AIV bombs).

Sleet01 · 2024-06-09T02:56:10Z

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java

+        double completeWeight = loadoutMap.values().stream().mapToDouble(curWeight -> Math.max(curWeight, 1.0)).sum();
+        double randomThreshold = (Compute.randomInt(100) / 100.0) * completeWeight;
+        for (String curMap : loadoutMap.keySet()) {
+            countWeight += Math.max(loadoutMap.get(curMap), 1.0);
+            if (countWeight >= randomThreshold) {
+                mapName = curMap;
+                break;
+            }
+        }


This randomly chooses which entry loadout map type to apply to the unit by summing the relative weights in the loadout map, then checking a random value somewhere between 0.0 and (sum of weights) against a cumulative sum of the weights again? And this works?

Just when I thought I couldn't hate arbitrary relative weighting any more...

My gut feeling is that this isn't going to work as planned, because bare Maps don't guarantee insertion-order ordering of entries. So e.g. for bombMapPirateGroundSpread, a randomThreshold of <= 3.0 is equally likely to produce "Normal" or "Firestorm", and a value of 7.1+ will either select "Firestorm" or "Normal" depending on the order in which entries are returned; the chance of returning "Firestorm" is actually higher than the relative weighting of 7 and 3 would suggest.

When a collection's ordering is not guaranteed it must be treated as if ordering is random, at least for modelling behaviour, so this approach seems unlikely to work correctly.
Unfortunately I'm not an expert in this field so I'd need to go do some research to figure out the ideal approach, but I would say that switching the maps to LinkedHashMaps... unfortunately this requires using something other than Map.ofEntries() to populate them. Perhaps a custom class that supports ofEntries()...

Actually, looking at the code below that uses this selected string, I don't see why we couldn't just create array lists that give us weighted xBombLoad mappings, without the role names that are only used for the switch cases.

e.g., instead of bombMapGroundSpread plus a bunch of complicated weighted-option-selecting code plus a switch on mapName, just cut out the middleman and do:

public static ArrayList<Map<Integer, Integer>> createWeightedList(Map<Map<Integer, Integer>, Integer> weightMap) { // For each map M, there will be N entries where N is the arbitrary weight (integer) ArrayList<Map<Integer, Integer>> entriesByWeight = new ArrayList<Map<Integer, Integer>>(); for(Map.Entry<Map<Integer, Integer>, Integer> m: weightMap.entrySet()) { // Create a sublist of size N = relative weight ArrayList<Map<Integer, Integer>> subList = new ArrayList<Map<Integer, Integer>>(m.getValue()); for (int i=0;i<subList.size();i++) { // Fill with references to the specified Map of BombType:Weight subList.add(m.getKey()); } // Add all entries entriesByWeight.addAll(subList); } return entriesByWeight; }

Pass it a Map of Maps and weights, like:

private static final Map<String, Integer> standardBombMapsMap = Map.ofEntries ( Map.entry(normalBombLoad, 6), Map.entry(antiMekBombLoad, 3), Map.entry(antiConvBombLoad, 2), Map.entry(standoffBombLoad, 1), Map.entry(strikeBombLoad, 2) ); private static final ArrayList<Map<Integer, Integer>> standardBombWeightedList = createWeightedList(standardBombMapsMap);

Then a randomized integer between 0 and the length of the resulting ArrayList can be used to randomly select one of the maps:

Map<Integer, Integer> curMap = standardBombWeightedList.get(Compute.randomInt(standardBombWeightedList.size());

Which is all kind of verbose, but now you can just get the maps and the probability of getting a given map equals (weight / sum of weights).

I've stepped through the code several dozen times during the development and testing process, and it does seem to produce reliable results at least as far as is intended.

As for the proposed substitute, that's getting very complicated, to the point where I'm having trouble following it. The (switch) is wordier and less elegant but also less opaque.

Don't want to put too much effort into fiddling with this, as eventually those distribution numbers are going to get pushed out to external data so players can fiddle with it to suit their needs.

Sleet01 · 2024-06-09T07:24:48Z

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java

+        Map<Integer,Integer> bombMap;
+        if (!isPirate) {
+            if (!airOnly) {
+                switch (mapName) {


This can all be significantly simplified.

Not certain how. The test cannot be combined, as it needs to split between pirate and non-pirate loads, then it also needs to choose between air/ground and air/air loads - didn't feel that with only two choices of air/air loads, and the limited ordnance choices warranted an extra set of maps.

Granted, I could remove the negation, but the coding style is to put more likely choices at the top so when stepping through or manually reading code, large jumps to following lines are outliers.

Sleet01 · 2024-06-09T07:29:16Z

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java

+            completeWeight = workingBombMap.values().stream().mapToDouble(curWeight -> Math.max(curWeight, 1.0)).sum();
+            randomThreshold = (Compute.randomInt(100) / 100.0) * completeWeight;
+            countWeight = 0.0;
+            for (int curBomb : workingBombMap.keySet()) {


See comments on similar block that selects specific maps from the map of roles and weights.

Sleet01

Apologies for making a bunch of corrections; I was re-acquainting myself with some of the base code while doing the review.

In general I like this approach, and it's similar to what I'm doing for generalized munition loadout creation. But I have some concerns and suggestions.

At a high level:

the random selection system is suspect. I don't believe it will actually select items using the weights specified, due to vagaries of the Java Map class. I think we can get by with turning Maps of the various BombType: weight mappings into ArrayLists and selecting using random indexes.
the mixing of bomb load mutation and application rubs me the wrong way; it should probably be two separate functions, or at least two separate and distinct blocks of code. There should really only be one call to setBombChoices for each unit.
some of the larger conditionals could be shrunk by creating collections of the Homing ammunition types, and checking for membership.

This looks like a significant improvement over the existing stuff, and I look forward to seeing it in action!

…ic list as constant.

… Push unarmed bombers to the front of the list so they always get loaded.

…an directly over Map key values. Ordnance tables now assigned directly rather than using an intermediate name. Some loop optimization.

SuperStucco · 2024-06-10T21:45:58Z

OK, those last few commits should cover most of what is requested. Not going to get into anything fancier on the weighted random selection, it looks to be working as intended so optimization can be done later.

Sleet01 · 2024-06-28T12:46:49Z

@SuperStucco Apologies, I totally missed the re-review request. I will take a look as soon as I can.

Sleet01

LGTM

Sleet01 · 2024-06-28T17:44:04Z

Check failures are for Gradle version mismatch, should be good to go.

SuperStucco added 6 commits June 4, 2024 16:56

Initial work on generating range of bombs

3c53a4e

Rebuild method for adding bombs to fighters. This cuts out the separa…

f80b9cd

…te method previously used.

Remove rockets from normal and anti-Mech loadouts

1e0f742

Add quality parameter to bomb generator method

d8b8107

General code work completed

2fe0190

Move static data to top of file, rebuild bomb number tracking to acco…

8d9aafc

…modate (undocumented!) input requirements for setting bombs on Entity.

Sleet01 self-requested a review June 6, 2024 22:27

IllianiCBT reviewed Jun 7, 2024

View reviewed changes

IllianiCBT added the AtB label Jun 7, 2024

Sleet01 reviewed Jun 9, 2024

View reviewed changes

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java Show resolved Hide resolved

Sleet01 reviewed Jun 9, 2024

View reviewed changes

MekHQ/src/mekhq/campaign/mission/AtBDynamicScenarioFactory.java Outdated Show resolved Hide resolved

Sleet01 reviewed Jun 9, 2024

View reviewed changes

Sleet01 requested changes Jun 9, 2024

View reviewed changes

SuperStucco added 4 commits June 10, 2024 09:57

Extracted check for guided ordnance to separate method. Now uses stat…

3acb98b

…ic list as constant.

Javadoc description updates

3e63346

Extract TAG addition to a separate method, clean up ordnance loading.…

af838df

… Push unarmed bombers to the front of the list so they always get loaded.

Change weighted random selection to iterate index of values rather th…

c334aa7

…an directly over Map key values. Ordnance tables now assigned directly rather than using an intermediate name. Some loop optimization.

Throw exception on unexpected rating

28e80b1

SuperStucco mentioned this pull request Jun 12, 2024

Add basic force generator role functionality to MekHQ scenario random unit generation #4214

Closed

SuperStucco requested a review from Sleet01 June 15, 2024 14:01

IllianiCBT added the For New Dev Cycle label Jun 21, 2024

Sleet01 approved these changes Jun 28, 2024

View reviewed changes

Sleet01 merged commit c680b93 into MegaMek:master Jun 28, 2024
1 of 4 checks passed

IllianiCBT mentioned this pull request Jul 15, 2024

Added Basic Force Generator Role Functionality to Scenario Random Unit Generation #4418

Merged

SuperStucco deleted the Bomb-mapping branch July 18, 2024 01:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More detailed auto-selection of bombs #4164

More detailed auto-selection of bombs #4164

SuperStucco commented Jun 6, 2024

IllianiCBT commented Jun 6, 2024

Sleet01 commented Jun 6, 2024

Sleet01 commented Jun 6, 2024

IllianiCBT commented Jun 6, 2024

SuperStucco commented Jun 6, 2024

IllianiCBT Jun 7, 2024

gsparks3 Jun 7, 2024 •

edited

Loading

SuperStucco Jun 7, 2024

IllianiCBT Jun 7, 2024

IllianiCBT Jun 7, 2024

SuperStucco Jun 7, 2024

IllianiCBT Jun 7, 2024

IllianiCBT commented Jun 7, 2024

HammerGS commented Jun 8, 2024

Sleet01 Jun 9, 2024

Sleet01 Jun 9, 2024 •

edited

Loading

SuperStucco Jun 10, 2024 •

edited

Loading

Sleet01 Jun 9, 2024 •

edited

Loading

Sleet01 Jun 9, 2024 •

edited

Loading

Sleet01 Jun 9, 2024

SuperStucco Jun 10, 2024

Sleet01 Jun 9, 2024

SuperStucco Jun 10, 2024

Sleet01 Jun 9, 2024

Sleet01 left a comment

SuperStucco commented Jun 10, 2024

Sleet01 commented Jun 28, 2024

Sleet01 left a comment

Sleet01 commented Jun 28, 2024

		(generatedBombs[BombType.B_LG] > 0 \|\|
		generatedBombs[BombType.B_HOMING] > 0);

More detailed auto-selection of bombs #4164

More detailed auto-selection of bombs #4164

Conversation

SuperStucco commented Jun 6, 2024

IllianiCBT commented Jun 6, 2024

Sleet01 commented Jun 6, 2024

Sleet01 commented Jun 6, 2024

IllianiCBT commented Jun 6, 2024

SuperStucco commented Jun 6, 2024

Choose a reason for hiding this comment

gsparks3 Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IllianiCBT commented Jun 7, 2024

HammerGS commented Jun 8, 2024

Choose a reason for hiding this comment

Sleet01 Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

SuperStucco Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Sleet01 Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

Sleet01 Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sleet01 left a comment

Choose a reason for hiding this comment

SuperStucco commented Jun 10, 2024

Sleet01 commented Jun 28, 2024

Sleet01 left a comment

Choose a reason for hiding this comment

Sleet01 commented Jun 28, 2024

gsparks3 Jun 7, 2024 •

edited

Loading

Sleet01 Jun 9, 2024 •

edited

Loading

SuperStucco Jun 10, 2024 •

edited

Loading

Sleet01 Jun 9, 2024 •

edited

Loading

Sleet01 Jun 9, 2024 •

edited

Loading