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

feat(gameState): Add relevant players culling native #2998

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

Conversation

Identity-labs
Copy link
Contributor

Goal of this PR

The game has a hard cap on displaying culled players (128 for FiveM, 32 for RedM).
This native allows server owners to control which players can be seen by others.
Since the culling radius does not prioritize displaying the closest players, this native allow server owners to implement their own logic to filter culled players.

How is this PR achieving the goal

Add native to set players net ids list for each players to be culled

This PR applies to the following area(s)

Server

Successfully tested on

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.

@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label Dec 13, 2024
void SET_PLAYER_CULLING_RELEVANT_PLAYERS(char* playerSrc, int playerTarget);
```
Sets the culling relevent players list for the specified player.

Choose a reason for hiding this comment

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

"Relevent" typo

@AvarianKnight
Copy link
Contributor

This reveals internal bits to the client which is already very bad since it would break expected behavior down the line if any of this ever changes, but on top of that this code is all wrong, which makes me thing that it wasn't actually tested.

@Identity-labs
Copy link
Contributor Author

The purpose of this PR is to have feedback on what can be done. I already have open PR before https://github.com/citizenfx/fivem/pull/1896/files with non constructive discussion.
Tested but not in production since it was tested without adhesive
Keep in mind c++ is not my main language

@tens0rfl0w
Copy link
Contributor

I understand why some might find a functionality like this useful. However, I don't believe it's a viable solution to the core issue of the limited number of players a client can process.

While the internal overhead introduced by the bit-tests might be acceptable, the overhead in ScRTs could be problematic. For instance, if someone uses this feature to implement custom culling with prioritization for players near a target client, it would necessitate high-frequency updates and checks. This would involve invoking different natives for each player, which could significantly impact performance on servers with higher player counts (where such functionality would be most needed). Given that these servers often already struggle to maintain the target tick rate, this approach could potentially only worsen the situation.

@Identity-labs
Copy link
Contributor Author

The main issue is to not slow down the sync thread to prioritise culled players. you have more advanced knowledge on the subject. How we can achieve this decently ? Can we try in production with beta build like It’s done in experimental forum ?

@PichotM
Copy link
Contributor

PichotM commented Dec 14, 2024

It will most likely create ownership issues for entities controlled by "out-of-scope" players
Just like what the culling natives often create btw

@Identity-labs
Copy link
Contributor Author

We already have ownership issue with curling radius, and we deal with it with workarounds

@AvarianKnight
Copy link
Contributor

That isn't exactly a good reason to add something else that could lead to more unknown/weird issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Needs a preliminary assessment to determine the urgency and required action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants