#4133 Field of View Bug

[Ryo]

ver: 0.37.11-exp

When the mech positioned on the roof of the Lv3 buiding hexes in City Sububs (2707 to 2709), the Field of View cannot function correctly. Please see the attached file for the exact locations of these hexes.

Thank you!

[Ryo]

In this check, I did not use both of Dead Zone Rule and Diagramming Line of Sight Rules.

[Nicholas Walczak]

Fixed in [r10896]. The issue was that building elevation wasn't being taken into consideration. The code had been computing the attacker and target absolute heights, which really isn't necessary. I changed it to use Entity.absHeight(). This is really the way I should have done it originally, but didn't (until now).

[saginatio]

The fix broke darken field of view feature:(

      ai.attackAbsHeight = ai.attackHeight

                + selectedEntity.elevationOccupied(game.getBoard().getHex(src));

Last line was used by fov when selectedEntity was in a hex with different elevation than src hex.

Sample save attached

[Nicholas Walczak]

Yea, this is likely why I originally wrote it that way and when I made the fix I was feeling lazy and thought I saw an easy way out. Should've realized my mistake.

[Nicholas Walczak]

Alright, might have this finally fixed in up [r10923].

BoardView1 keeps a list of StepSprites that it uses for drawing the movement path. Each StepSprite keeps a reference of the MoveStep its representing. If there's a selected unit, the attackAbsHeight is now calculated as the selected Entity's height plus its elevation if there are no StepSprites, otherwise, the elevation is taken from the MoveStep of the last StepSprite.

[saginatio]

The bug is still there. I'm testing on trunk rev 10923, there's a sample save attached.

If you can give me couple of minutes to think about it, I can try to diagnose the problem and find a fix. I'm currently working near BoardView1.getLosEffects, so it won't be much trouble for me.

[saginatio]

Ok, I found it and fixed it:

if (selectedEntity != null) {
        ai.attackHeight = selectedEntity.getHeight();
        int elevation;
        if (pathSprites.size() > 0) {
            // If we've got a step, get the elevation from it
            MoveStep lastMS= pathSprites.get(pathSprites.size() - 1).getStep();
            elevation = lastMS.getElevation();
            //if !lastMS.getPosition().equals( src ) there is something fishy going on
            //its best to precise method's javadoc, or declaration
            //assert lastMS.getPosition().equals( src );
        } else {
            //otherwise we use entity's elevation and
            elevation = selectedEntity.getElevation();
        }
        ai.attackAbsHeight = srcHex.getElevation() + elevation + selectedEntity.getHeight();
    }

The direct cause of this bug was that ai.attackAbsHeight value (and methods that use ai.attackAbsHeight) expects to be set to absolute elevation, i.e. the number of levels attacker is above a level 0 hex. But instead it was set to a relative elevation.

In my humble opinion of an outside programer who tries to understand MegaMek's code: The indirect cause is
i) the lack of javadoc around some AttackInfo fields.
ii) very strange naming of Entity.absHeight():

If we have an entity e that is an attacker placed in a hex with elevation!=0 ,
then if we construct proper AttackInfo ai object for e,
then the following is true:

    ai.attackAbsHeight != e.absHeight()

Which is veeeery counterintuitive.
I hope I was able to word my thoughts in comprehensive way:)

I will clear my code of unnecessary comments, add javadoc in places I see its most needed and post a patch soon(tm).

Edit: I've added a patch with fix and some javadoc in LosEffects, Entity and IHex;
Edit2: I've attached a wrong file, the proper one is resent.

[saginatio]

I have a second version of patch, same fix, same javadoc, but additionally I've refactored absHeight() and renamed it to getElevationOfHighestPoint() (I know the name is not fortunate, but it is the best I could think of now, and this is just a demonstration.)
Thanks to this change I found some probable bugs in the code:
Server line:15888

    if ((target != null)
            && (target.getTargetType() == Targetable.TYPE_ENTITY)) {
        // Lets re-write around that horrible hack that was here before.
        // So instead of asking if a specific location is wet and praying
        // that it won't cause an NPE...
        // We'll check 1) if the hex has water, and 2) if it's deep enough
        // to cover the unit in question at its current elevation.
        // It's especially important to make sure it's done this way,
        // because some units (Sylph, submarines) can be at ANY elevation
        // underwater, and VTOLs can be well above the surface.
        te = (Entity) target;
        IHex hex = game.getBoard().getHex(te.getPosition());
        if (hex.containsTerrain(Terrains.WATER)) {
--->        if (te.absHeight() < hex.getElevation()) {
                damage = (int) Math.ceil(damage * 0.5f);
            }
        }
    }

hex.getElevation() returns absolute elevation of surface, while te.absHeight() returns relative one (sic!).

WeaponAttackAction line 387 & 2098

    if (te == null) {
        targEl = game.getBoard().getHex(target.getPosition()).floor();
    } else {
        targEl = te.absHeight();
    }

hex.floor() returns absolute elevation of floor, ... so targetEl has either absolute or relative elevation stored in depending on if choice.

WeaponHandler 1262

    double atkLev = ae.getElevationOfHighestPoint();
    double tarLev = entityTarget.getElevationOfHighestPoint();
    double levDif = Math.abs(atkLev - tarLev);
    String hexType = "LASER inhibiting smoke";

    // loop through all intervening coords.
    // If you could move this to compute.java, then remove - import
    // java.util.ArrayList;
    for (Coords curr : coords) {
        // skip hexes not actually on the board
        if (!game.getBoard().contains(curr)) {
            continue;
        }
        ITerrain smokeHex = game.getBoard().getHex(curr)
                .getTerrain(Terrains.SMOKE);
        if (game.getBoard().getHex(curr).containsTerrain(Terrains.SMOKE)
                && wtype.hasFlag(WeaponType.F_ENERGY)
                && ((smokeHex.getLevel() == 3) || (smokeHex.getLevel() == 4))) {

            int levit = ((game.getBoard().getHex(curr).getElevation()) + 2);

            // does the hex contain LASER inhibiting smoke?
            if ((tarLev > atkLev)
                    && (levit >= ((travel * (levDif / range)) + atkLev))) {
                refrac++;
            } else if ((atkLev > tarLev)
                    && (levit >= (((range - travel) * (levDif / range)) + tarLev))) {
                refrac++;
            } else if ((atkLev == tarLev) && (levit >= 0)) {
                refrac++;
            }
            travel++;
        }
    }

To be honest I dont know what this particular method is doing but it just seems wrong :/

HexTarget

public int absHeight() {
    return getHeight() + getElevation();
}


public int getElevation() {
    return m_elev;
}

public HexTarget(Coords c, IBoard board, int nType) {
    m_coords = c;
    m_elev = board.getHex(m_coords).getElevation();
    m_type = nType;
    m_bIgnite = (nType == Targetable.TYPE_HEX_IGNITE);
}

I'm not sure about this one either but getElevation in targetable should return relative elevation not an absolute one.

I dont expect you to substitute some other name for absHeight() because I understand it could be quite tedious in a multibranch repository. I also know, by a rule of thumb, that it is usualy a bad decision in a semiestablished api with external references to do so.
Still if by any chance that method got refactored, it would make me a happy man :). (and if you want more happines for me, then you could rename IHex.getElevation to IHex.getLevel or IHex.getAbsElevationOfSurface :P )

[Nicholas Walczak]

I've been slowly working on getting your patch pulled in, but I've had a lot of distractions.

I wholeheartedly agree with you on the naming conventions we use for height and elevation as it's tripped me up numerous times (as is witnessed with this ticket).

[Nicholas Walczak]

As an aside, this is also an excellent example of where we should have unit tests...

[Nicholas Walczak]

I decided to commit the first patch for two reasons. One, I had already applied it and was modifying it by the time I saw this, and two I'm not very keen on the new name for absHeight(). I think that the API should be renamed, but I'm not sold on this particular. This is also a change that should go into exp and not trunk.

One of the problems with absHeight is that it's not truely absolute height, it's actually relative height since it's Entity.getElevation() (which is the elevation the entity is above the hex it's standing in) + Entity.getHeight().

Personally, I think that all three methods should be renamed.
The method names end up confusing concepts and a lot of the words end up meaning the same thing. I think it's a discussion that needs to happen amongst several people to get things right.

[saginatio]

I agree that this need renaming needs to be thoroughly thought, maybe a separate ticket so that other developers can come with their ideas. My proposition was just a concept proof. As for naming convention I would suggest borrowing bit from TW:

-"level" would mean hex' surface/floor elevation above a "lvl 0" hex and only that. (the only other legal use of word level would be for as a unit for height parameter)
-"elevation" would mean relative distance to surface
-"absElevation" would mean distance to a surface of a level 0 hex
-"height/depth" would be reserved for measuring how tall entities are.

major changes would be:

hex.getElevation -> hex.getLevel
entity.absheight -> e.getTopElevation
attackInfo.absHeight -> ai.absTopElevation  (topAbsElevation)

[Nicholas Walczak]

Could you create the ticket with relevant info? Put it into the post-stable milestone. You can also list me as the owner.

[saginatio]

ok, I'll take care of that a bit later.

[Nicholas Walczak]

I also think that a separate bug ticket should be created for the 4 questionable pieces of code. The purpose of that code needs to be determined, and a test case needs to be created showing that the code works or not, and then the code needs to be fixed and the test cases then need to succeed. It's actually probably a good idea to create 4 separate tickets.

[Nicholas Walczak]

Ok, a slightly modified version of this first patch has been committed in [r10931]. I ended up using IHex.surface() instead of IHex.getElevation(), since I think it makes more sense in this case (the values are the same).