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

One issue #1

Closed
quiret opened this issue Nov 17, 2021 · 7 comments
Closed

One issue #1

quiret opened this issue Nov 17, 2021 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@quiret
Copy link

quiret commented Nov 17, 2021

Found issues in version 1.1.0

  • if there are two peds synced for the player and both have the same custom model assigned to them, then if one of them streams out your syncer attempts to release the custom model for both of the ped elements, especially the one already streamed in. While the documentation of engineFreeModel does not mention what happens in this case, I assume that the request fails. This scenario is a potential model ID leak (the client will run out of available real model IDs at some point). Even worse: the client could run out of RAM very quickly.

Moved suggestions to: #3

https://forum.mtasa.com/topic/133212-rel-add-new-models-library/#comment-1003406

@Fernando-A-Rocha
Copy link
Owner

Fernando-A-Rocha commented Nov 17, 2021

Thank you for the challenges, sounds really fun! Will let you know what I decide to work on asap.

@Fernando-A-Rocha Fernando-A-Rocha self-assigned this Nov 17, 2021
@Fernando-A-Rocha Fernando-A-Rocha added bug Something isn't working enhancement New feature or request labels Nov 17, 2021
@Fernando-A-Rocha
Copy link
Owner

  • if there are two peds synced for the player and both have the same custom model assigned to them, then if one of them streams out your syncer attempts to release the custom model for both of the ped elements, especially the one already streamed in.

I can confirm this issue does indeed happen. I forgot to make it check when an element streams out if there is need for freeing the model. There should only be need for freeing the model if no other streamed elements are using the same model.

  • While the documentation of engineFreeModel does not mention what happens in this case, I assume that the request fails. This scenario is a potential model ID leak (the client will run out of available real model IDs at some point). Even worse: the client could run out of RAM very quickly.

engineFreeModel only takes a model ID argument and frees that model ID, leaving all elements that had it with the model ID defined serverside when they were spawned (e.g. CJ skin when a ped was created). This in itself doesn't create model ID leaks as it frees the models properly. Please note that in no point in the client script I store the elements in an array, only the model IDs are stored, which are used by that function.

I will address the fact that the model is being freed incorrectly on streamOut, which can leave other elements of the same time with reset models if they had the same allocated ID.

@quiret
Copy link
Author

quiret commented Nov 17, 2021

engineFreeModel only takes a model ID argument and frees that model ID, leaving all elements that had it with the model ID defined serverside when they were spawned (e.g. CJ skin when a ped was created). This in itself doesn't create model ID leaks as it frees the models properly. Please note that in no point in the client script I store the elements in an array, only the model IDs are stored, which are used by that function.

I see. In your script you seem to be assigning model IDs on the clientside only. But are you aware that server-side model IDs do not necessarily match client-side model IDs? You are not mentioning that peds with free'd model IDs are being reset to CJ on the clientside, which actually is the argument I made. So if engineFreeModel does not reset peds to CJ on the clientside, then it indeed is a resource leak. How about you check whether engineFreeModel returns false? This could be a very important hint for the user regarding resource leaks (there could be a spurious one if another resource uses your internally allocated model ID in another clientside script).

EDIT: After testing the engineFreeModel function myself I have found out that it resets the model ID of all peds to 0 if it matches the to-be-released model ID. This does not match the standard which would say PSYCHO ped skin by default. But it does match what you have said for the serverside.

@Fernando-A-Rocha
Copy link
Owner

I've found another issue: when a model ID is changed on an element detected via onClientElementDataChange, it does not check if the oldValue (aka old model ID) is no longer needed. I'm gonna make it so it frees it if no longer needed, same behavior when an element streams out.

Fernando-A-Rocha added a commit that referenced this issue Nov 17, 2021
Issues/Behaviors fixed:

 - scenario: two elements exist with the same model. one is streamed out. the script frees the model ID without verifying there is still a second element using that model, ending up resetting its model to the one defined serverside on creation

- scenario: player's skin is changed from a custom model to another custom model: script doesn't free the old skin ID used, occupying memory space

- 'player' element type was incorreclty being changed to 'ped' all the time [fix: they are two distinct types except when used in engineRequestModel argument]
@Fernando-A-Rocha
Copy link
Owner

@quiret Updated: Can you download the resource again and let me know how your testing goes now?

@Fernando-A-Rocha
Copy link
Owner

Also could you please put your suggestions separately into other issues? Thank you! 🙏

@quiret quiret changed the title One issue and suggestions One issue Nov 17, 2021
@quiret
Copy link
Author

quiret commented Nov 17, 2021

The original issue has been fixed. Good work!

@quiret quiret closed this as completed Nov 17, 2021
@Fernando-A-Rocha Fernando-A-Rocha removed the enhancement New feature or request label Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants