Fix getPedMoveState returns incorrect move states

[Unde-R]

This should fix the following issues: #1598 #1594 #1590

[Lpsd]

Thanks for your PR. I'm sure this will be fine but I'll need to do some extensive testing later just to make sure it doesn't unintentionally break anything else. Good work though!

[Unde-R]

Thanks for your PR. I'm sure this will be fine but I'll need to do some extensive testing later just to make sure it doesn't unintentionally break anything else. Good work though!

Thanks mate

[Unde-R]

I tested a code to get groundposition and compared it with player's Z position in the areas which return "fall" and here is the results ( Z - groundposition ) : 1,00000095367 1.00000095367 1.00000095368

[qaisjp]

thanks for submitting this. quick question for future documentation / reproduction purposes, please can you provide the coordinates you tested with? thank you :)

[Unde-R]

thanks for submitting this. quick question for future documentation / reproduction purposes, please can you provide the coordinates you tested with? thank you :)

sure one moment

[Unde-R]

Ground Position: 14.250931739807
Player Position: 2221.7590332031, -1653.4652099609, 15.250932693481
-------------
Ground Position: 14.252799987793
Player Position: 2221.814453125, -1653.5106201172, 15.252800941467
-------------
Ground Position: 14.287864685059
Player Position: 2223.142578125, -1653.3934326172, 15.287865638733

[Unde-R]

I added MOVEMENTSTATE_ROLL because It was declared but no code for It.

• [x] crouch-roll
• [x] crawl-roll

Result: https://pastebin.com/vz4zQb3D

[Einheit-101]

This is absolutely amazing!

[Unde-R]

Looks like this issue should be fixed too #534

[Unde-R]

Thanks for the help! Tested It's working fine now.

[qaisjp]

The suggested code I provided is not correct. Please read https://github.com/multitheftauto/mtasa-blue/pull/1603#discussion_r467206465

[Unde-R]

The suggested code I provided is not correct. Please read #1603 (comment)

return (vecPosition.fZ > fGroundLevel && (vecPosition.fZ - fGroundLevel - 1.0f) <= ((fabs(vecPosition.fZ) < fabs(fGroundLevel) ? fabs(fGroundLevel) : fabs(vecPosition.fZ)) * FLOAT_EPSILON)); Thoughts on this?

[qaisjp]

Almost there! Remember, there's two checks here. One that checks vecPosition.fZ > fGroundLevel, and another that checks (vecPosition.fZ - fGroundLevel) <= 1.0f.

I think this should be split up into two if-s, with correct float comparisons.

To be honest, the code is a bit hard to read, so perhaps you should add helpers like these* to Utils.cpp or Utils.h.

Then you can do something like this:

    // We don't care if the ped is underground
    if (definitelyGreaterThan(vecPosition.fZ, fGroundLevel))
        return false;
        
    // Check (vecPosition.fZ - fGroundLevel) <= 1.0f
    return definitelyLessThan(vecPosition.fZ - fGroundLevel, 1.0f) || essentiallyEqual(vecPosition.fZ - fGroundLevel, 1.0f);

I'm not sure if this best practice, but it's the best thing I can come up, and I feel like it's better than just adding 0.001 and calling it a day.

[*] this link is from "Swift", but it is actually some chat software, and not the Swift Programming Language

[Unde-R]

    // We don't care if the ped is underground
    if (definitelyGreaterThan(vecPosition.fZ, fGroundLevel))
        return false;
        
    // Check (vecPosition.fZ - fGroundLevel) <= 1.0f
    return definitelyLessThan(vecPosition.fZ - fGroundLevel, 1.0f) || essentiallyEqual(vecPosition.fZ - fGroundLevel, 1.0f);

I tried this ^^ but somehow It's not working properly.

[Unde-R]

// We don't care if the ped is underground
    if (definitelyGreaterThan(vecPosition.fZ, fGroundLevel))
        return false;

Looks like It's because of this.

[Pirulax]

Do this (like in the CVector class):

abs(vec.z) - abs(groundposz) >= FLOAT_EPSILON

This is totally acceptable, as float epsilon is a thing in CS

[qaisjp]

    // We don't care if the ped is underground
    if (definitelyGreaterThan(vecPosition.fZ, fGroundLevel))
        return false;

The code I suggested had a small mistake.

It doesn't work because we're returning false if our z-position is greater than the ground level. That does not match up to the comment that says "we don't care if the ped is underground"?

It should return false if the ped is underground. But we're not checking if the ped is underground correctly.

you should use definitelyLessThan instead :)

[Unde-R]

Yeah I'll update it soon. What about crouch-roll should I keep it here or make another PR for it?

[qaisjp]

What about crouch-roll should I make another PR for it?

Good idea

[Unde-R]

Good idea

I'll open a PR rn

[Einheit-101]

This is tested a lot and seems to be fine, why is this not merged yet?

[patrikjuvonen]

This is tested a lot and seems to be fine, why is this not merged yet?

No accepted reviews, and no testing beyond the stale ones has been made. I am interested to hear if this actually resolves the three linked issues and how does it do that since it is not apparent from the description. This leaves me to have to thoroughly delve into the code to figure out whats going on, how to test it and how to try to break it. None of which I have time to do right now personally.

[Einheit-101]

I could test functionality with a working build if i can get one. My resource uses these functions.

[ds1-e]

@patrikjuvonen

Tested on nightly (Multi Theft Auto v1.5.9-release-21251 - but the version shouldn't matter, since it exists on stable as well)

#1598 - fixed build & nightly build #1594 - fixed build & nightly build #1590 - fixed build & nightly build

Besides those locations, i've tested whether it happens in the other places, which haven't been mentioned. And it seems that fix works well.

[patrikjuvonen]

@patrikjuvonen

Tested on nightly (Multi Theft Auto v1.5.9-release-21251 - but the version shouldn't matter, since it exists on stable as well)

#1598 - fixed build & nightly build #1594 - fixed build & nightly build #1590 - fixed build & nightly build

Besides those locations, i've tested whether it happens in the other places, which haven't been mentioned. And it seems that fix works well.

I think you shared the wrong nightly build video for 1598, it's the same as 1594. Tests should be equal.

[patrikjuvonen]

I don't think 1594 is fixed by this PR (based on the video above). Why is it returning false for move state when the move state is supposed to be something specific? It will still be causing the same headache.

[Unde-R]

I don't think 1594 is fixed by this PR (based on the video above). Why is it returning false for move state when the move state is supposed to be something specific? It will still be causing the same headache.

because of this https://github.com/multitheftauto/mtasa-blue/pull/1603/files#diff-25862afc0e3088d4619017944046a0d75c9a75a9ffbdb666216bb3a9478453fdR2381

[Unde-R]

I don't think 1594 is fixed by this PR (based on the video above). Why is it returning false for move state when the move state is supposed to be something specific? It will still be causing the same headache.

basically in the video of the fixed build, he was standing at first then he's trying to enter the vehicle thats why its showing false, because he's entering the vehicle

        else if (IsEnteringVehicle())
            return MOVEMENTSTATE_UNKNOWN;

[patrikjuvonen]

Exactly. I don't understand why it has been specified as unknown. If the ped is jogging to enter a vehicle, shouldn't it return jogging?

[ds1-e]

@patrikjuvonen i guess qaisjp agreed upon making it false

He also suggested it:

Vehicle-related movement states

Note that getPedMoveState documents what happens when the player is in a vehicle:

    Returns a string indicating the ped's move state, or false if the ped is not streamed in, the movement type is unknown, the ped is in a vehicle or the ped is invalid.

Basically, I'm not 100% sure whether enter_vehicle and leave_vehicle fits well with the rest of these move states. What do you think?