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

Refactor: Complete Script Rewrite #65

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Mycroft-Studios
Copy link

@Mycroft-Studios Mycroft-Studios commented Jul 16, 2024

Describe Pull request
A complete Rewrite of the script, into a modular based system, benefits of the rewrite include:

  • Much easier to read
  • Fully documented code
  • Easy to follow each part
  • Security improvements
  • Performance improvements

    from my testing, it goes to 0.02ms max, and that is only for a single tick while dropping off the passenger, the rest of the time it will stay at 0.00ms

  • Better coding practices
  • Removes almost all threads
  • Removes unused Code

Questions (please complete the following information):

  • Have you personally loaded this code into an updated qbcore project and checked all it's functionality? Yes
  • Does your code fit the style guidelines? Yes
  • Does your PR fit the contribution guidelines? Yes

A complete Rewrite of the script, into a OOP based system,
benefits of the rewrite include:
- Much easier to read
- Fully documented code
- Easy to follow each part
- Security improvements
- Performance improvements (from my testing, it goes to 0.02ms max, and that is only for a single tick while dropping off the passenger)
- Better coding practices
- Removes all threads
credit to Mutt for noticing this :)
@Jamie9192
Copy link

Nicely structured and documented

Ive only scanned through the server lua - a performance improvement would be:

Server.object = exports['qb-core']:GetCoreObject() removed

local GetPlayerObject = exports['qb-core']:GetCoreObject().Functions.GetPlayer caching the single function, instead of the full core.

and call this function to get the players object :)

Copy link
Contributor

@Z3rio Z3rio left a comment

Choose a reason for hiding this comment

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

Just nitpicking.
Great PR mate.

client/modules/npc.lua Outdated Show resolved Hide resolved
client/modules/bus.lua Outdated Show resolved Hide resolved
Comment on lines 18 to 20
'@PolyZone/EntityZone.lua',
'@PolyZone/CircleZone.lua',
'@PolyZone/ComboZone.lua',
Copy link
Contributor

Choose a reason for hiding this comment

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

Only CircleZone is used, no?
I see no usage of EntityZone, BoxZone, ComboZone atleast

Copy link
Author

Choose a reason for hiding this comment

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

maybe?
it has been a good while since i have used PolyZone, so didnt want to break it lol

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah dont really know personally either, if the CircleZone perhaps depends on one of the other imported files.
If not though, then they should probably be removed to improve performance / overhead, no?

Copy link
Author

Choose a reason for hiding this comment

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

then they should probably be removed to improve performance / overhead, no?

indeed. will do some testing and see if the others are required :)

@Mycroft-Studios
Copy link
Author

Nicely structured and documented

Ive only scanned through the server lua - a performance improvement would be:

Server.object = exports['qb-core']:GetCoreObject() removed

local GetPlayerObject = exports['qb-core']:GetCoreObject().Functions.GetPlayer caching the single function, instead of the full core.

and call this function to get the players object :)

Fixed :)

while this might sound like a weird change, onResourceStart does not seem to be 100% reliable in current builds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants