Fix #3310: Improved client-side markers & Fix logic

[Nico8340]

This PR aims to fix the client-side logic of markers (events only got called for players), and it also creates two new events, onClientPlayerMarkerHit and onClientPlayerMarkerLeave. Closes #3310.

[FileEX]

Why do we need an additional event separately for the player?

[Nico8340]

Why do we need an additional event separately for the player?

To match the server side events. This will not break backwards compatibility, because the onClientMarkerHit event is also called when players hit the markers.

[PlatinMTA]

You would need to fix all the scripts that use onClientMarketHit if this solves #3310. If I'm reliable expecting only players and I get, let's say, a vehicle, then my script will break.

Do we actually need to change how that event works? The solution is simple.

local mrk = createMarker(0, 0, 3)
local col = getElementColShape(mrk)

function markerHit(element, dim)
    if not dim then
        return
    end
    outputChatBox(inspect(element).." entered the marker")
end
addEventHandler("onClientColShapeHit", col, markerHit)

[Nico8340]

You would need to fix all the scripts that use onClientMarketHit if this solves #3310. If I'm reliable expecting only players and I get, let's say, a vehicle, then my script will break.

Do we actually need to change how that event works? The solution is simple.

local mrk = createMarker(0, 0, 3)
local col = getElementColShape(mrk)

function markerHit(element, dim)
    if not dim then
        return
    end
    outputChatBox(inspect(element).." entered the marker")
end
addEventHandler("onClientColShapeHit", col, markerHit)

Yes, because this event should have worked like this for 20 years, this is rather a bug, not an enhancement. By the way, it is not only triggered on the vehicle, but separately on the player, so getVehicleController is not needed, and this PR creates two more events, which can be used to register only the player's hit.

[PlatinMTA]

Yes, because this event should have worked like this for 20 years, this is rather a bug, not an enhancement.

Why? Where's the proof? Since 2008 the wiki states that marker events are for players only. Markers for players only makes sense (this also triggers when you are inside a vehicle btw), why would a marker need to be triggered when an object goes into contact with it? For that just use a colshape (or even better, the marker's colshape). Colshapes don't even have a player only event. I'm pretty sure this isn't an error nor a bug, just the intended way for this to work.

By the way, it is not only triggered on the vehicle, but separately on the player, so getVehicleController is not needed,

I don't understand this. Colshape events trigger once for the player and then once for the vehicle, so eitherway you need to use getVehicleController or getPedOccupiedVehicle, the information is not shared. Or does this event send the vehicle/player as an argument apart from the original element that hit the colshape? Something like:

Arguments: element hitElement, boolean matchingDimension, element vehicle/table vehicleOccupants

and this PR creates two more events, which can be used to register only the player's hit.

At most if we are going to change the functionality of this event we shouldn't create a new one just for the players. Leave the one that already exists and create a new one for general purposes, or just change the one that already exists and do not add a new one. I would prefer to do neither and leave it the way it is, here's why:

The problem is that we are just duplicating the event calls. Now not only we have the colshape event and the marker hit event, but we added a new marker hit event just for the sake of it. Now every time any element gets into the colshape of a marker it will trigger two events instead of one, and every time a player hits that same marker it will trigger three events instead of two. This is just clut. Even worse, scripters that use onClientMarkerHit (which arent that many to be fair) now need to change their scripts to add a new check for element type or to change the event that they were already using because it changed names, which is not really nice.

So I don't really get this change at all, the functionality that the main issue was asking is already there. The reviewer of this PR should consider this information first before merging.

[Nico8340]

Why? Where's the proof? Since 2008 the wiki states that marker events are for players only. Markers for players only makes sense (this also triggers when you are inside a vehicle btw), why would a marker need to be triggered when an object goes into contact with it? For that just use a colshape (or even better, the marker's colshape). Colshapes don't even have a player only event. I'm pretty sure this isn't an error nor a bug, just the intended way for this to work.

The answer to this question is simple. In the basic game, the markers are compatible with all type of elements, and the event on the server side also works in the same way, the client side event was "corrected" based on this model.

I don't understand this. Colshape events trigger once for the player and then once for the vehicle, so eitherway you need to use getVehicleController or getPedOccupiedVehicle, the information is not shared. Or does this event send the vehicle/player as an argument apart from the original element that hit the colshape?

No, this event does not currently have such a function, and I consider it completely unnecessary, I probably worded this in a misunderstanding way, I'm sorry.

At most if we are going to change the functionality of this event we shouldn't create a new one just for the players. Leave the one that already exists and create a new one for general purposes, or just change the one that already exists and do not add a new one. I would prefer to do neither and leave it the way it is, here's why:

As I mentioned earlier in my answer, this PR is modeled after the event server-side counterpart.

The problem is that we are just duplicating the event calls. Now not only we have the colshape event and the marker hit event, but we added a new marker hit event just for the sake of it. Now every time any element gets into the colshape of a marker it will trigger two events instead of one, and every time a player hits that same marker it will trigger three events instead of two. This is just clut.

This is how it is intended to work, for the reason you just mentioned: If someone needs to modify their existing code after this PR gets merged, they can do it much more easily, there is no need to perform any checks manually.

Even worse, scripters that use onClientMarkerHit (which arent that many to be fair) now need to change their scripts to add a new check for element type or to change the event that they were already using because it changed names, which is not really nice.

This is not necessary for the reasons mentioned above, although I consider it as a standard, because the server-side equivalent of this event is triggered for every element, I think most developers check the element type by default.

[Nico8340]

One more thing I missed: You mentioned that onClientMarkerHit is also triggered for vehicles. This is not the case in the current implementation, but this PR also fixes this. In the current implementation, there is an if, which prevents all other element types except the player from passing.

I think it's completely pointless to use the collision of the marker in Lua, because even though it really works this way, it's not the intended way.

[PlatinMTA]

You mentioned that onClientMarkerHit is also triggered for vehicles

I mention that with this PR that change would be done and hence you will need to change your scripts to minimize triggering events for other elements. I was searching the wiki history and apparently onMarkerHit also only triggered for players only but it was changed around 2011 (so somewhere around 1.0.4?~). I wonder why it was changed considering onColShapeHit already existed.

I understand your reasoning to make this change, but I think this is unnecessary clutter we should avoid, and unnecessary hassle for the few scripters that are already expecting only players into their events. A fair compromise would be to make a new event instead of changing the one we already had working this way. I'm still against that change, but I think that's the best of both worlds.

(with source being element) onClientElementMarkerHit or the counterpart (source being marker) onClientMarkerElementHit

But that's all I wanted to say

[Proxy-99]

so what is the state of this pull request @Nico8340 is it backwards compatible? I think it should not affect current scripts

[Nico8340]

so what is the state of this pull request @Nico8340 is it backwards compatible? I think it should not affect current scripts

It is not fully backwards compatible, because if the necessary checks have not been carried out, then by default only a player element is expected, and this can disrupt the expected operation.