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

Add new on(Client)ElementCreated event #218

Closed
wants to merge 23 commits into from
Closed

Add new on(Client)ElementCreated event #218

wants to merge 23 commits into from

Conversation

CrosRoad95
Copy link
Contributor

@CrosRoad95 CrosRoad95 commented Jul 4, 2018

Client side:
name: onClientElementCreated
1st argument: type: boolean; true if element was created by server, false if created by client
source is created element.

Server side:
name: onElementCreated
no arguments.
source is created element.

working with all types of elements from https://wiki.multitheftauto.com/wiki/Element . If i something missed, please mention.

General test of almost every element type.
test1.zip

Some script that collect every created element by type and return them.
test2.zip

This PR closes #389

@botder
Copy link
Member

botder commented Jul 9, 2018

What will happen when a 1 MB map with 6000+ objects gets loaded? I assume the event will get spammed. You probably don't want that and you could use on(Client)ResourceStart. When you manually create objects then you already know when it's created. What is the purpose/usage for this event?

@botder botder added enhancement New feature or request in progress labels Jul 9, 2018
@Dezash
Copy link
Contributor

Dezash commented Jul 9, 2018

on(Client)ElementDestroy was a mistake too in my opinion. The only use for it is to debug something and we already have debug hooks for that. If we keep adding events for every single function, MTA performance will eventually suffer.

@botder botder added new-feature feedback Further information is requested and removed enhancement New feature or request labels Jul 9, 2018
@CrosRoad95
Copy link
Contributor Author

What will happen when a 1 MB map with 6000+ objects gets loaded? same thing is with current onElementDestroy function and even bigger spam with onElementDataChange

@qaisjp qaisjp requested a review from ccw808 July 22, 2018 20:02
@qaisjp
Copy link
Contributor

qaisjp commented Jul 22, 2018

Requesting review from @ccw808 to evaluate performance implication.

@patrikjuvonen patrikjuvonen added enhancement New feature or request and removed new feature labels Sep 4, 2018
@patrikjuvonen patrikjuvonen changed the title event: on(Client)ElementCreated Add new on(Client)ElementCreated event Sep 4, 2018
@patrikjuvonen patrikjuvonen assigned ccw808 and ghost Sep 4, 2018
@Pirulax
Copy link
Contributor

Pirulax commented Oct 14, 2018

What will happen when a 1 MB map with 6000+ objects gets loaded? same thing is with current onElementDestroy function and even bigger spam with onElementDataChange

Well, not, because onElementDataChange event isn't triggered for map elements, take a look at the code.

@AlexTMjugador
Copy link
Member

When you manually create objects then you already know when it's created. What is the purpose/usage for this event?

In a production server with many resources, it might not be desirable or feasible to use exports or custom events to tell everyone who wants to know that elements were created. Creating exports for just that purpose may not be considered elegant (it kinda pollutes the resource public interface with things that are more appropiate to handle as events), and custom events might have the problem of event name collisions. For instance, in a roleplay server, a car shop resource might fire an "onVehicleCreate" event to indicate that it spawned a car for sale, but so can do another resource which loads cars from a database.

Of course, the evident and naive solution to the custom event name collision problem is to modify resources so they use a different name. And handling element creation as custom events makes sense, so it's elegant too. But you, as a server owner, might not be able to do that if one of the affected resources is compiled, and even though compiling scripts is not really intended as a way to protect against script tampering, it can effectively become that.

So right now the only other approach left to act on response of the creation of an element, assuming that exports are not used and custom events are not feasible due that (admitely) corner case of one of the resources being compiled and not modifiable, are debug hooks. Which are, obviously, very slow and not designed for that. In the end, given these circumstances, it is possible to realize that it would be nice to have some convenient feature inside MTA to do that without resorting to debug hooks, which are unelegant for this problem and slow. That convenient feature could be this event, because even though it could be slow, its semantics might correspond better with those of the problem that we want to solve and anyway it should be faster than debug hooks.

However, to encourage responsible use of this event from scripters, I think that the wiki page associated to it should clearly state what performance problems it could have, and the highest possible source element of the event should be a child of the root element, to avoid it being fired for literally everything and still be an useful tool to handle this case.

All in all, my opinion is that even though there could be well justified reasons for the existence of this event, it's not a very important extra feature to have. It's possible to get around its absence by other methods, as stated above.

@botder botder added this to the Backlog milestone Mar 4, 2019
@ArranTuna
Copy link
Collaborator

The only argument against is performance yet just use addDebugHook and see the gigantic spam: onClientRender, onClientPreRender, onClientHUDRender most players will be on 60 FPS so that's 180 events per second right there. On a busy server you'll have onClientWorldSound, onClientPedStep, onClientPlayerWeaponFire, onClientPlayerDamage, onClientVehicleDamage, onClientElementDataChange all of these will be getting called so much that the performance impact of this would be negligible. In fact if onClientElementStreamIn exists there is no reason why onClientElementCreated shouldn't exist since streaming in actually like the creation of the element.

crun t1 = getTickCount() for i=1, 100000 do triggerEvent("abc", localPlayer) end t2 = getTickCount() outputChatBox(t2 - t1)

Calling triggerEvent 100,000 times only took 35 miliseconds and that's through Lua function call, so surely the only relevant performance impact that events have is when they're actually being handled?

This event should be added because it goes with the resource based system and especially with a community based resource system. I'll think of an example:

A server has 10 different resources that creates ped. For Halloween they want all peds on the server to become a group of zombie skins. Now how would I go about achieving that? Well I could use onClientElementStreamIn but that's no good because I want a random zombie skin picking, but I want each player to actually see the same skins as everyone else, so something needs doing server side. I'd either have to edit every script and add onElementCreated manually or I'd have to have a timer constantly checking for new elements being created. So actually adding this event wouldn't decrease performance, it would INCREASE performance by reducing the need for hacky scripts and definitely save us time adding hacky scripts.

In fact there's another event that would be spammy like this but increase performance: onClientElementEnterScreen so we wouldn't need to for loop everything and check is on screen which is certainly inefficient. This would benefit all scripts that render text in the 3D world like above players heads.

@qaisjp qaisjp removed the request for review from ccw808 September 3, 2019 14:59
@qaisjp
Copy link
Contributor

qaisjp commented Sep 3, 2019

This event should be added because it goes with the resource based system and especially with a community based resource system. I'll think of an example:

A server has 10 different resources that creates ped. For Halloween they want all peds on the server to become a group of zombie skins. Now how would I go about achieving that? Well I could use onClientElementStreamIn but that's no good because I want a random zombie skin picking, but I want each player to actually see the same skins as everyone else, so something needs doing server side. I'd either have to edit every script and add onElementCreated manually or I'd have to have a timer constantly checking for new elements being created. So actually adding this event wouldn't decrease performance, it would INCREASE performance by reducing the need for hacky scripts and definitely save us time adding hacky scripts.

Honestly this is the best example you could have come up with

@qaisjp
Copy link
Contributor

qaisjp commented Sep 3, 2019

Is there a cleaner way to do this instead of doing CallEvent everywhere? i.e. using managers

Copy link
Contributor

@qaisjp qaisjp left a comment

Choose a reason for hiding this comment

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

It would be good if we could come up or discuss a cleaner solution than adding this event everywhere

@Pirulax
Copy link
Contributor

Pirulax commented Nov 13, 2020

Do not add this event.
Calling event handlers for everything is slow. Very slow.
I have tested it, but this issue: #1067 confirms it.
The destroy event has its own overhead already, lets not add another step of it.
Im pretty sure there was a reason not to add it in the first place.

@ghost
Copy link

ghost commented Nov 27, 2020

Almost nobody seems to be in favor of this. If you can come up with a different solution, you can create a new PR.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feedback Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new on[Client]ElementCreate event
9 participants