Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "fix(handling-loader): fix broken physics" #3134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jrgrimshaw
Copy link

@jrgrimshaw jrgrimshaw commented Feb 6, 2025

Goal of this PR

As outlined in detail in #3130, the following PR attempts to 'validate' against NULL values when using SetVehicleHandlingFloat or SetVehicleHandlingInt to prevent vehicle physics from breaking. The issue is that NULL values are parsed as 0, meaning that you can also no longer use the value 0 in these natives (which is common).

Furthermore, the issue the PR was intending to fix wasn't actually caused by NULL values; it's isolated to the handling loader setting a high value for fTractionCurveMax when 0.0f/NULL is detected (source).

Not allowing values of 0 breaks many scripts, including my own. Even if NULL is used, it is parsed as 0 anyway, and therefore the commit doesn't protect against anything & doesn't fix the root issue. You can see this by taking a look at CheckArgument; in the case of setting a float, it checks whether the value is equal to an empty constructor of the type, which in the case of a float would be like running float(), which returns 0.0f.

Furthermore, the issue the PR was intending to fix wasn't actually caused by NULL values; it's isolated to the handling loader setting a high value for fTractionCurveMax when 0.0f/NULL is detected (source)

How is this PR achieving the goal

Reverts #3101 to allow 0 (same as NULL) values again.

This PR applies to the following area(s)

FiveM

Successfully tested on

Game builds: 3258

Platforms: Windows, Linux

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

Fixes issues

fixes #3130

@github-actions github-actions bot added the invalid Requires changes before it's considered valid and can be (re)triaged label Feb 6, 2025
@github-actions github-actions bot added triage Needs a preliminary assessment to determine the urgency and required action invalid Requires changes before it's considered valid and can be (re)triaged and removed invalid Requires changes before it's considered valid and can be (re)triaged triage Needs a preliminary assessment to determine the urgency and required action labels Feb 6, 2025
@CeebDev
Copy link

CeebDev commented Feb 6, 2025

It is not the core codebase's responsibility to prevent bad or untested scripting

I dont 100% agree with that, if you go that way, so in every native we just dont do any check on args passed.
But if NULL is parsed as 0.0f in these fields, then it's a GTA problem and I agree.

Imo we should have atleast a message when we pass NULL

@DaniGP17
Copy link
Contributor

DaniGP17 commented Feb 6, 2025

Imo we should have atleast a message when we pass NULL

I'm not sure if that's possible, but I do think it's a good idea to have a warning when 0.0 is applied in some handlings, which is the same as null in this case, because it will set a very large value that breaks the physics.

if (fTractionCurveMax == 0.0f)
{
*(float*)(handlingChar + 140) = 100000000.f;
}

@jrgrimshaw
Copy link
Author

It is not the core codebase's responsibility to prevent bad or untested scripting

I dont 100% agree with that, if you go that way, so in every native we just dont do any check on args passed.

But if NULL is parsed as 0.0f in these fields, then it's a GTA problem and I agree.

Imo we should have atleast a message when we pass NULL

I tested a decent range of different handling values and null/0.0 produce the same effect.

I took a look at CheckArgument to confirm & this is correct. In the case of setting a float, it checks whether the value is equal to an empty constructor of the type, which in the case of a float would be like running float(), which returns 0.0f.

So right now, we are validating against using 0.0f. So for example if you wanted to reset a vehicle's suspension height with SetVehicleHandlingFloat(vehicle, "CHandlingData", "fSuspensionRaise", 0.0), this currently results in an error.

@CeebDev
Copy link

CeebDev commented Feb 6, 2025

Imo we should have atleast a message when we pass NULL

I'm not sure if that's possible, but I do think it's a good idea to have a warning when 0.0 is applied in some handlings, which is the same as null in this case, because it will set a very large value that breaks the physics.

if (fTractionCurveMax == 0.0f)
{
*(float*)(handlingChar + 140) = 100000000.f;
}

So this is the problem, why set this at 100000000.f and other handling values not ?

@DaniGP17
Copy link
Contributor

DaniGP17 commented Feb 6, 2025

I'm not sure if that's possible, but I do think it's a good idea to have a warning when 0.0 is applied in some handlings, which is the same as null in this case, because it will set a very large value that breaks the physics.

if (fTractionCurveMax == 0.0f)
{
*(float*)(handlingChar + 140) = 100000000.f;
}

So this is the problem, why set this at 100000000.f and other handling values not ?

Well that's the same behaviour of the game.
image

@Cocodrulo
Copy link

Smth I dont understand, why a handling parameter cannot be set with 0.0 or 0? What's the reason?

@DaniGP17
Copy link
Contributor

DaniGP17 commented Feb 7, 2025

Smth I dont understand, why a handling parameter cannot be set with 0.0 or 0? What's the reason?

Well that depend in the handling but if you look in the else of the screenshot I send you can see a division, if you divide by 0.0 ... 🤯

@Cocodrulo
Copy link

But using the native I get an error

@DaniGP17
Copy link
Contributor

DaniGP17 commented Feb 7, 2025

But using the native I get an error

That's the reason to revert the previous PR, because the code is considering 0.0 as null, if this PR is merged we'll get the previous behavior that is allowing the 0.0 values.

@Cocodrulo
Copy link

Got it

@Cocodrulo
Copy link

Or at least, don't allow null, but allow 0.0 and 0

@DaniGP17
Copy link
Contributor

DaniGP17 commented Feb 7, 2025

Or at least, don't allow null, but allow 0.0 and 0

#3134 (comment)

@jrgrimshaw
Copy link
Author

Any update on this? If the issue is spread to the stable version of the client it's going to screw up a lot of handling-related scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Requires changes before it's considered valid and can be (re)triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when using SetVehicleHandling natives with value of 0
4 participants