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

Reorganize KVP natives and fix Lua example for the "find" functions #2511

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

Mathu-lmn
Copy link
Contributor

Goal of this PR

This PR puts all the KVP related native decls in the kvp folder
It also provides a fix for the Lua example script that wasn't working due to a wrong boolean check and a missing Wait

How is this PR achieving the goal

By moving all the KVP files in the KVP folder and fixing the Lua example in both StartFindKvp and StartFindExternalKvp

This PR applies to the following area(s)

Natives

Game builds: X

Platforms: X

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.

@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label May 10, 2024
@FabianTerhorst FabianTerhorst removed the request for review from gottfriedleibniz August 15, 2024 06:32
Copy link
Contributor

@AvarianKnight AvarianKnight left a comment

Choose a reason for hiding this comment

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

These don't need a Wait, doing so would just slow down getting the KVPs (especially server side, since it ticks once every 50ms).

ext/native-decls/kvp/StartFindKvp.md Outdated Show resolved Hide resolved
ext/native-decls/kvp/external/StartFindExternalKvp.md Outdated Show resolved Hide resolved
@Mathu-lmn
Copy link
Contributor Author

Have you tried it ? I can't right now as I'm away but I can next week if needed !

@AvarianKnight
Copy link
Contributor

Yes, the example works fine, you don't need to wait to get the kvp string though.

@Mathu-lmn
Copy link
Contributor Author

I wonder why I added it then. I think the client crashed with no wait but I can't check now

@AvarianKnight
Copy link
Contributor

Likely crashed with the until key because there would never be a key so it would be an infinite loop

@Mathu-lmn
Copy link
Contributor Author

That's why I changed it to until not key 😬

@Mathu-lmn
Copy link
Contributor Author

But wouldn't the client crash in a loop with no wait ?

@AvarianKnight
Copy link
Contributor

AvarianKnight commented Aug 15, 2024

But wouldn't the client crash in a loop with no wait ?

The old loop (which was until key) would crash with a endless loop because it would wait for key to be valid, which it would never be.

The fixed loop (which is until not key) can't crash because if there isn't a key it will break out of the loop.

@Mathu-lmn
Copy link
Contributor Author

Yes, what I meant is that in regular while loops, if you don't put a Wait(0), the Fivem client freezes and you ultimately crash. I'm wondering if it's the case with the repeat operator too

@Mathu-lmn
Copy link
Contributor Author

I've tested without the Wait and it works fine, thanks for the feedback @AvarianKnight

Copy link
Contributor

@FabianTerhorst FabianTerhorst left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. 👍

@FabianTerhorst FabianTerhorst added ready-to-merge This PR is enqueued for merging and removed triage Needs a preliminary assessment to determine the urgency and required action labels Aug 19, 2024
@prikolium-cfx prikolium-cfx merged commit 17382d9 into citizenfx:master Aug 20, 2024
6 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.

4 participants