issue_334 Better direction choices based on angle

[balloonpopper]

Corrects unexpected walking angles seen in the previous commit based off my testing. Also preferences more animation angles so it should handle 8, 4 or 2 direction animations.

[stickgrinder]

@balloonpopper I really like the idea of testing the direction angle, instead of the sector; also great use of the wrapf function to bring some sanity to the -180:179 Godot standard.

You also solved the face_SIDE functions in a simpler and better way than I would have done! :+1: :smiley: Great!

The only thing that I would change is the nested if/else block. It's barely readable to me, and the cyclomatic complexity is pretty high. I would write it in a flatter style, going "round-the-clock" like this:

if angle < 22.5:
    _animation_suffixes = ['_r', '_dr', '_l', '_dl', '_d']
    _looking_dir = Looking.RIGHT
elif angle < 45:
    _animation_suffixes = ['_dr', '_r', '_dl', '_l', '_d']
    _looking_dir = Looking.DOWN_RIGHT
elif angle < 67.5:
    _animation_suffixes = ['_dr', '_d', '_dl', '_r', '_l']
    _looking_dir = Looking.DOWN_RIGHT
elif angle < 90:
    _animation_suffixes = ['_d', '_dr', '_dl', '_r', '_l']
    _looking_dir = Looking.DOWN
elif angle < 112.5:
    _animation_suffixes = ['_d', '_dl', '_dr', '_l', '_r']
    _looking_dir = Looking.DOWN
elif angle < 135:
    _animation_suffixes = ['_dl', '_d', '_dr', '_l', '_r']
    _looking_dir = Looking.DOWN_LEFT
elif angle < 157.5:
    _animation_suffixes = ['_dl', '_l', '_dr', '_r', '_d']
    _looking_dir = Looking.DOWN_LEFT
elif angle < 180:
    _animation_suffixes = ['_l', '_dl', '_r', '_dr', '_d']
    _looking_dir = Looking.LEFT
elif angle < 202.5:
    _animation_suffixes = ['_l', '_ul', '_r', '_ur', '_u']
    _looking_dir = Looking.LEFT
elif angle < 225:
    _animation_suffixes = ['_ul', '_l', '_ur', '_r', '_u']
    _looking_dir = Looking.UP_LEFT
elif angle < 247.5:
    _animation_suffixes = ['_ul', '_ur', '_u', '_l', '_r']
    _looking_dir = Looking.UP_LEFT
elif angle < 270:
    _animation_suffixes = ['_u', '_ul', '_ur', '_l', '_r']
    _looking_dir = Looking.UP
elif angle < 292.5:
    _animation_suffixes = ['_u', '_ur', '_ul', '_r', '_l']
    _looking_dir = Looking.UP
elif angle < 315:
    _animation_suffixes = ['_ur', '_ul', '_u', '_r', '_l']
    _looking_dir = Looking.UP_RIGHT
elif angle < 337.5:
    _animation_suffixes = ['_ur', '_r', '_ul', '_l', '_u']
    _looking_dir = Looking.UP_RIGHT
else:
    _animation_suffixes = ['_r', '_ur', '_l', '_ul', '_u']
    _looking_dir = Looking.RIGHT

WARNING: I didn't double check the values in this, yet, it's just a quick example, please take a look!

Also, I need to review the fallback arrays: can you please point me out if you changed them, how and why? EDIT: what I'm asking here is related to your claim of the former result feel less "natural", but I see you used the same angles after all. So how did you address the problem and what has changed in practice?

Thanks a lot, great work!

[balloonpopper]

@stickgrinder I agree that yours is more readable. I was trying to optimise it from (worst case) 16 if statements down to 4, but happy to prioritise readability / maintainability. Modified accordingly.

I made some changes to my previous commit to my fallback arrays, I believe they are correct now. I think there's some differences in my preference choices, and also some bugs in the original code.

I've only traced out the first section of the original code, this may be the only bug, or the might be more. Below is the code from L765 to L770 with the variables replaced by the values to make it easier to read: https://github.com/carenalgas/popochiu/commit/80c645999e14ec7d2960dbe716b3fc70aa40e6c2#diff-099209ce75040aaa85afb7f28211f97fbbaa3499bb39c0e9dae1b98df6705da7R765

    elif angle >= (22.5) and angle < (67.5):
        _looking_dir = Looking.DOWN_RIGHT
        if angle >= (45) and angle < (135):
            _animation_suffixes = ['_dr', '_r', '_dl', '_l']
        else:
            _animation_suffixes = ['_dr', '_dl', '_d']

This check "if angle >= (45) and angle < (135):" will work, but the 135 is the wrong value. If anything it should've been 67.5 - but only the >=45 check is actually needed.

Then in relation to my angle preferences vs yours: For an angle of 60 degrees, the original code shown above will preference R if DR is unavailable (['_dr', '_r', '_dl', '_l']). 60 is closer to 90 so it should prefer down.

[balloonpopper]

Additionally in relation to : https://github.com/carenalgas/popochiu/issues/340 As a test I added this code where the rad_to_deg angle is calculated:

        # Top of file has "var _last_direction_faced:int = -1"
        ########################################

    <...>

    var angle = wrapf(rad_to_deg((destination - position).angle()), 0, 360)
    var direction_faced = int(angle/45)
    if direction_faced == _last_direction_faced:
        return
    _last_direction_faced = direction_faced

It fixes the code having to constantly reevaluate the facing direction, but I believe the whole walking subsystem should likely be optimised (see issue 340). If you don't agree with 340, but think the above code is helpful, I can add it to the PR.

[stickgrinder]

This check "if angle >= (45) and angle < (135):" will work, but the 135 is the wrong value. If anything it should've been 67.5 - but only the >=45 check is actually needed.

You are right about that being a problem; the original code was very hard to read and it's way better now anyway, thanks!

Then in relation to my angle preferences vs yours: For an angle of 60 degrees, the original code shown above will preference R if DR is unavailable (['_dr', '_r', '_dl', '_l']). 60 is closer to 90 so it should prefer down.

That's ok to me! Being a matter of preferences, the best option is trying it in the wild and see if someone raises some complaints :) If not, your version just works :magic_wand:

About #340, I totally agree. That code is very legacy and my personal take is that it grew over time and has never been re-evaluated because to a modern machine that's trivial stuff anyway. BUT that's not a good reason to keep inefficiency in code, so: great catch.

What I would do, if you agree is: review and merge this PR as it is, if you consider it ready, and ask you to open a dedicated one for #340 with all the optimization you mentioned.

What do you think?

[balloonpopper]

Happy for you to merge the code as is if you're happy with it. It has the history if you want this discussion to be able to refer back to how it developed. If you want to keep the commits down though let me know and i will squash it to a single commit.

[stickgrinder]

No need to squash manually, gh can do this during the merge.

This discussion makes enough sense tracing back the most important parts :) Thanks!