0.49.10: Regression: VTOLs can get stuck in swamp

[mhjacks]

(See #3237) - it appears that since the fix was merged the groundingVTOL variable is gone, and it is possible for VTOLs to become stuck in swamp hexes again.

Environment

Standalone MegaMek 0.49.10, running under Linux JDK java-11-openjdk-11.0.17.0.8-1.fc37.x86_64

Description

See above referenced bug

Files

stuck_vtol.sav.gz

In the sav file, the stuck VTOLs are Karnov BA models and a Vector. Unlike the previous iteration of the bug, the VTOLs "unstuck" themselves the next round, but the two Karnovs were strafed to death by enemy Hydaspes.

[HammerGS]

Don’t think this is a regression. We have other reports since that fix. Seems to be issue flying over negative terrain.

[HammerGS]

Related - #3499

[mhjacks]

Here is a repro case on latest master embedded in mekhq.
autosave.sav.gz

It doesn't happen every time, and it isn't necessarily negative terrain.

Looking at the code:

public PilotingRollData checkBogDown(MoveStep step,
            EntityMovementType moveType, Hex curHex, Coords lastPos,
            Coords curPos, int lastElev, boolean isPavementStep) {
        PilotingRollData roll = getBasePilotingRoll(moveType);
        int bgMod = curHex.getBogDownModifier(getMovementMode(),
                this instanceof LargeSupportTank);

It seems like flying VTOLs should return auto-success for bgMod. Is it fair to ask why they don't?

[mhjacks]

In Terrain.java, this seems odd:

        // hovercraft and WiGE don't bog down. Naval units never touch the floor and thus don't bog down.
        if (moveMode.isHoverOrWiGE() || moveMode.isNaval()) {
            return TargetRoll.AUTOMATIC_SUCCESS;
        }

        switch (type) {
            case Terrains.SWAMP:
                // if this is quicksand, then you automatically fail
                if (level > 1) {
                    return TargetRoll.AUTOMATIC_FAIL;
                } else if (moveMode.isVTOL()) {
                    return TargetRoll.AUTOMATIC_FAIL;
                } else {
                    return 0;
                }

This seems like it might be counter-intuitive if I read it right (and it's quite possible I'm not). Auto-fail for VTOL movement, auto-fail if level > 1 (i.e. flying over the terrain in question?)

[pheonixstorm]

In that instance level is the terrain level and not the elevation of the unit. iirc level 1 is swamp and level 2 is quicksand. I would have to look it up to make sure, but woods has three levels, so if you saw level > 1 it would just be pointing to heavy and ultra heavy woods.

[mhjacks]

Ah, so this is related to the "Can't occupy heavy woods" restrictions. That makes sense. Thanks!

I noticed there is also an isHoverVTOLOrWiGE method; but I suspect this won't quite work because it is technically possible for a VTOL to get stuck while landed. There also seems to be a isAirborneVTOLorWIGE that might be helpful here.

I don't know what's been explored; and I have to think some of these things have been considered

[SuperStucco]

In that instance level is the terrain level and not the elevation of the unit. iirc level 1 is swamp and level 2 is quicksand. I would have to look it up to make sure, but woods has three levels, so if you saw level > 1 it would just be pointing to heavy and ultra heavy woods.

Might be an idea for some code clean-up to have a better variable name to improve clarity and avoid future confusion for those without historical/in-depth knowledge of that code block. I suspect something similar is the ultimate cause of the issue, with a reasonably-sounding but incorrect variable name is being confused in place of another.

[mhjacks]

OK, that's fair. I'm astounded at how convoluted some of these calculations are (that's not a criticism, I'm impressed how much of it works all of the time). I know that can wind up being a very deep rabbit hole, though.

What do you think of this theory though - the isHoverOrWige seems like it may be the wrong thing to check in moveMode, since it's not clear that VTOLs have "Hover" for movement mode (they seem to have "VTOL"; but again I could very well be reading this wrong). What if the auto-success test adds something like "isHoverOrWigeOrVTOL && not on "floor" level"?

[mhjacks]

OK - back in Entity.java we have:

if ((!lastPos.equals(curPos) || (step.getElevation() != lastElev))
                && (bgMod != TargetRoll.AUTOMATIC_SUCCESS)
                && (moveType != EntityMovementType.MOVE_JUMP)
                && (floorLevelGroundUnit || groundingVTOL) && !isPavementStep) {

Conceptually this looks right - I have to wonder if something is causing something we don't expect in floorLevelGroundUnit or groundingVTOL. They both check getElevation as I'd expect and by the implications of the variables they seem like they're checking the right things.

Is there a reasonable way to check Elevations during the game without tremendous amounts of log spam? It occurs that we might want to check Elevation directly in this test, but that's already being checked almost directly by both of these variables.

[HammerGS]

Posting some additional data from testing. Both of these are starting at level 1 flying over -1 terrain.

The VTOL above has no issues flying over the terrian.

In the second screen shot I immediately desend 1 elevation to level 0, flying over a -1 terrian. I should still be considered at an elevation of 1. 0-(-1)= Elevation 1.

You can see in the debugger that it's now treating this a groundingVTOL (landing) which then triggers the bog down check.

This save sets up the above scenario but with one VTOL starting at Elevation 1 the other at Elevation 0 but both over -1 Height Terrain.

Attached save sets up the above scenario.
Bog.sav.gz