induktio / thinker

Improved game engine features for SMACX.
https://discord.gg/XdFuwWzzku
GNU General Public License v2.0
75 stars 11 forks source link

Should it be dst, not src? #73

Closed tnevolin closed 4 months ago

tnevolin commented 5 months ago

https://github.com/induktio/thinker/blob/85c33fe5a78e5cffbcbf7638abe300d83ac7eefb/src/map.cpp#L202C54-L202C61

Fungus treated as road means stepping into fungus counts as a stepping along the road. In your formula, should it be dst square where fungus checked, not src?

tnevolin commented 5 months ago

In fact, unit does not need to start with road/base to benefit from fungus road. So your conditions should be reworked completely.

tnevolin commented 5 months ago

There seem to be few more problem with this function in other places.

1

if (is_ocean(sq_dst)) {
    if (bit_dst & BIT_FUNGUS
    && sq_dst->alt_level() == ALT_OCEAN_SHELF
    && Units[unit_id].triad() == TRIAD_SEA
    && unit_id != BSC_SEALURK // Bug fix
    && unit_id != BSC_ISLE_OF_THE_DEEP
    && !has_project(FAC_XENOEMPATHY_DOME, faction_id)) {
        return Rules->move_rate_roads * 3;
    }
    return Rules->move_rate_roads;
}

Is this to say non native sea unit spends 3 turns entering sea fungus? I think it is true regardless of the ocean depth. If I am right, the condition "&& sq_dst->alt_level() == ALT_OCEAN_SHELF" is too narrow.

2

My understanding is that parameter faction_id == -1 is very allowed and means "do not account for faction specific bonuses". However, do account for unit bonuses/abilities.

Again, if I am right, then all places where you implicitly uses it as boolean convert -1 to true. Is this what you intended?

induktio commented 4 months ago

Without very good reasons there should not be much need to change these original functions unless it is related to some specific bugfix, such as applying the faster movement also on Sealurks. It might even be possible some of the old code relies on inconsistent calculation on some values, so it would have to be tested separately. Most often the pathfinding code is called with a specific faction_id though.

About the other points the function should be working as intended, such as applying fungus effects only on tiles where it is actually rendered on ocean shelf. Furthermore if some of the details are original game mechanics we would need more reasons why they would have to be changed now.

tnevolin commented 4 months ago

Sure. I think you can keep it as is.