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

Rework camera shake toggle convar in FiveM #2360

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

Disquse
Copy link
Contributor

@Disquse Disquse commented Jan 24, 2024

Goal of this PR

The way how this functionality was implemented before is very fragile and got broken in b3095. The one-way nopping was also breaking the ability to disable this option after disconnecting from a server. So it was decided to rework it a bit.

How is this PR achieving the goal

By reworking the way how this functionality was implemented.

This PR applies to the following area(s)

FiveM

Successfully tested on

Game builds: 1604, 2060, 2189, 2372, 2545, 2612, 2699, 2802, 2944, 3095

Platforms: Windows

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

Addresses the problem reported in this forum topic (#3):
https://forum.cfx.re/t/a-couple-of-problems-since-the-last-updates/5200554

{
if (g_cameraShakeConvar->GetValue())
if (!g_cameraShakeConvar->GetValue())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like g_disableCameraShakeConvar would be a better name to reflect the value of this convar meaning. Since this is a rework - we can improve this as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better: how about not calling GetValue on the convar in the hook but instead using the tracking variable support so that there's only a single bool check?

The way how this functionality was implemented before is very fragile and got broken in b3095. The one-way nopping was also breaking the ability to disable this option after disconnecting from a server. So it was decided to rework it a bit.
@nihonium-cfx nihonium-cfx added the ready-to-merge This PR is enqueued for merging label Jan 29, 2024
@nihonium-cfx nihonium-cfx merged commit d6ea321 into citizenfx:master Jan 29, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR is enqueued for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants