-
-
Notifications
You must be signed in to change notification settings - Fork 529
Add new getElementsSyncedByPlayer
#4582
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
base: master
Are you sure you want to change the base?
Conversation
tederis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request!
isElementSyncer feedback
- I have concerns around the argument order of
isElementSyncerserver-side being confusing. Should the element be first, or the player be first? Currently in your pull request, the player is first. - What's the benefit of
isElementSyncerserver-side when you can already doif element.syncer == player? This is more concise and doesn't introduce confusion around the argument order.
I'd prefer we remove the isElementSyncer portion of this pull request.
getElementsSyncedByPlayer feedback
The filter feels strange to me. Do we have other examples of filters being parsed as comma-separated strings?
The linked issue didn't really explain why they want to filter by type, so for now I'd suggest just returning all the elements. We can always add filtering in a future pull request.
isElementSyncer for serverside && add new getElementsSyncedByPlayergetElementsSyncedByPlayer
|
Thanks for pointing this out! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good. Btw it would be good if you merged the CStaticFunctionDefinitions directly into the luadefs.cpp file. (And removed the assertion.)
The CStaticFunctionDefinitions exists mainly for historical reasons and is not necessary for new functions
…e unused declarations
done |
Closes #2400
This PR adds new
getElementsSyncedByPlayerwhich returns element table synced by player.Shared: