Fix the infinite loop of trying to spawn a blacklisted monster

[cknight828]

Summary

None

Purpose of change

Fix #75212

When trying to spawn monsters from a monster group, the quantity of monster to be spawn is not decreased when a sub group of top level monster group is selected, because it should be decreased in the sub group. But if some monsters are blacklisted and they are selected in the sub group, they are replaced with the default monster of the top level group but the quantity is not decreased.
For this, extra monsters may be spawn but it normally does not cause serious problems, because the valid monster in the sub group or the individual monster entry of the top level group is selected eventually in the loop. But at the worst case, the top level group contains only monster groups and all monsters in the group are blacklisted (like GROUP_VANILLA with Only Wildlife), the game will freeze.

Describe the solution

The quantity of monster is decreased when a sub group is selected but a blacklisted monster is seleceted in it.

Describe alternatives you've considered

Testing

Start game with Only Wildlife mod and reach FEMA camp.

Additional context

[RenechCDDA]

This actually changes spawn behavior quite significantly, take the following example:

GROUP, PICK ONE
---FULLY_BLACKLISTED_GROUP, weight 1
---NOT_BLACKLISTED_MONSTER, weight 1

Current behavior will be that the not blacklisted monster will always spawn, because the blacklisted group is excluded as a valid choice.

With your change, it's a 50% chance to spawn nothing at all.

A better approach would be loop detection, even a hacky solution of counting the number of times it's loop as some very high number (e.g. 100,000+) and then breaking if we hit that count.

[cknight828]

GROUP, PICK ONE
---FULLY_BLACKLISTED_GROUP, weight 1
---NOT_BLACKLISTED_MONSTER, weight 1

Current behavior will be that the not blacklisted monster will always spawn, because the blacklisted group is excluded as a valid choice.

No. The blacklisted group is not excluded in MonsterGroupManager::FinalizeMonsterGroups and still exists when spawning.
If FULLY_BLACKLISTED_GROUP is selected, an empty vector is returned from recursive call of GetResultFromGroup and the default monster of GROUP is added to result. Currently, the quantity of monster is not decreased in this case, so the default monster continues to be spawned until NOT_BLACKLISTED_MONSTER is spawned.

Is this behavior intended?

With your change, it's a 50% chance to spawn nothing at all.

Either of NOT_BLACKLISTED_MONSTER or default monster of GROUP is spawned.
I think it works well in local test.

[cknight828]

This PR fixes multi spawning monsters like place_monsters of mapgen.
Spawning once, like place_monster, works well currently because it doesn't check quantity.

[RenechCDDA]

No. The blacklisted group is not excluded in MonsterGroupManager::FinalizeMonsterGroups and still exists when spawning. If FULLY_BLACKLISTED_GROUP is selected, an empty vector is returned from recursive call of GetResultFromGroup and the default monster of GROUP is added to result. Currently, the quantity of monster is not decreased in this case, so the default monster continues to be spawned until NOT_BLACKLISTED_MONSTER is spawned.

Is this behavior intended?

It looks like your assessment is completely correct(!?). That is really, REALLY not how I believed it worked and appears to be contrary to how blacklists work with item groups.

I'm going to go ahead and say that behavior is a bug, but also you don't have to deal with it in this PR.

[RenechCDDA]

I think this is still 'double counting' the quantity? If we find a monster inside the for-loop, quantity is decremented. But then we exit the loop and decrease it again, regardless of whether or not we found a monster. The check should probably be

    if( quantity && !monster_found && is_recursive ) {
        ( *quantity )--;
    }

I would love to verify the behavior, but I have been unable to get place_monsters to spawn any monsters, at all. So I literally cannot test this.

[cknight828]

I think this is still 'double counting' the quantity? If we find a monster inside the for-loop, quantity is decremented. But then we exit the loop and decrease it again, regardless of whether or not we found a monster. The check should probably be

    if( quantity && !monster_found && is_recursive ) {
        ( *quantity )--;
    }

Quantity is not decremented when spawn_details is empty, so double counting does not occur. But monster_found is not set properly, it should be fixed and the check can depend on it.

I would love to verify the behavior, but I have been unable to get place_monsters to spawn any monsters, at all. So I literally cannot test this.

Spawning is guaranteed when density is set higher than 0.1

stale

[cknight828]

@RenechCDDA Is there something wrong? I found the same issue in 0.H and want to backport this pr.

[RenechCDDA]

It's just waiting on merger review right now. I don't personally have merge permissions.