open.mp icon indicating copy to clipboard operation
open.mp copied to clipboard

Regarding **abuse** of OnPlayerConnect and OnPlayerDisconnect for scripts

Open AmyrAhmady opened this issue 1 year ago • 29 comments

Just like what title says, this is an abuse, we are not limited nor restricted to stick with those events. If it was working back then in one 3rd party library, not used widely enough to be our base of opinions and implementations

Inconsistency & making zero sense:

image

"Call" OnPlayerDisconnect for connected players?! "Call" OnPlayerConnect for already connected players?

The amount of people reaching out to me because this breaks their single filterscript map file, or an admin script, or anything else is a lot, and the fact that it makes absolutely zero sense (player is already CONNECTED!).

Does player leave your server when you unload a filterscript? No! Does player just connect to your server when you try to load your filterscript? No!

So why are we making this breakable and unmanageable change to mess with people's scripts? How is it possible to distinguish whether player has just connected to your server, and it's not just a simple filterscript reload/load? How is any of this related to PLAYER'S CONNECTION? do we do anything on network level when a filterscript gets loaded? I sure hope not!

So what's the solution?

Create two new callbacks/events for that. you load a script, a new event dedicated to that cause gets called, like OnPlayerJoinScript, or OnScriptLoadForPlayer? Then keep OnPlayerDisconnect and OnPlayerConnect to what they are meant to be used for, to what they are created for, to what they are meant to, the meaning!

Final words, most important ones

This is an issue made for community, I'm asking anyone seeing this to give their opinion on this, consider this a community poll. If you have nothing to say, use numbers, use + or -, just say +1 or -1.

AmyrAhmady avatar Dec 12 '23 12:12 AmyrAhmady

+1

ksenonadv avatar Dec 12 '23 12:12 ksenonadv

+1

ReshiramZekrom1 avatar Dec 12 '23 12:12 ReshiramZekrom1

+1

pushline avatar Dec 12 '23 13:12 pushline

+1

PazzOnee avatar Dec 12 '23 14:12 PazzOnee

+1

NexiusTailer avatar Dec 12 '23 14:12 NexiusTailer

+1

selvagoat avatar Dec 12 '23 19:12 selvagoat

+1.

Not a fan of "hacky" workarounds

DobbysGamertag avatar Dec 12 '23 23:12 DobbysGamertag

+1

What about OnScriptUnloadForPlayer

adib-yg avatar Dec 13 '23 00:12 adib-yg

+1.

FreddieCrew avatar Dec 13 '23 06:12 FreddieCrew

I always support any innovations. But I think this is a strange proposal.

What's the point of these two callbacks?

I have never encountered such a problem and neither have everyone I know. Yes, this is no reason not to notice it, but adding these new callbacks is strange for all the reasons.

Although I'm not advocating not adding this, so +-1 ( :+1: :-1: ). The fact that this may "fix" the behavior of some people's scripts is a good thing. And I like that there is communication with people and this is discussed openly, and not closed. I hope it stays like this in the future :1st_place_medal:

shierru avatar Dec 13 '23 07:12 shierru

+1 sir I need hacktoberfest shirt

Hual avatar Dec 13 '23 08:12 Hual

+1

Zorono avatar Dec 13 '23 08:12 Zorono

+1 that makes sense

Hreesang avatar Dec 13 '23 10:12 Hreesang

+1

itsimperium avatar Dec 13 '23 12:12 itsimperium

+1 - I'm a bit with @shierru here, I don't necessarily think the new CBs are useful ; OTOH at least these won't break my scripts if I don't use them, so it's still a MASSIVE improvement over what we've got going on right now. :-)

Cheaterman avatar Dec 13 '23 16:12 Cheaterman

+1 rhis is a great suggestion and will be very useful

gHenriqueCarlos avatar Dec 13 '23 18:12 gHenriqueCarlos

I'll just leave here what I found out in 2017 and wrote about in 2020, so yeah I don't like this abuse either

References:

  • https://github.com/openmultiplayer/old.open.mp/issues/96
  • https://github.com/pawn-lang/sa-mp-fixes/issues/77

I really have a problem with this statement regarding OnPlayerDisconnect in a FilterScript: Think of it less like OnPlayerLeaveServer and more like OnScriptLoseAccessToPlayer.

What you did there, is changing the meaning of the callback. Now what that callback means is dependent on the type of script it is in (i.e. gamemode vs. filterscript). I get the point of the necessity of a callback with the meaning you proposed. However instead of changing the meaning of the original callback, you should add a new callback that implements this new meaning.

To clarify, there's a reason that OnGameModeExit can still be used in filterscripts next to OnFilterScriptExit, because each has its own distinct meaning and use. OnGameModeExit means exactly what it says, the gamemode that gets unloaded. OnFilterScriptExit means exactly what it says, the filterscript that gets unloaded.

In the same way, OnPlayerDisconnect and a new callback OnScriptLoseAccessToPlayer (or a better name) should have their own distinct meanings and uses. OnPlayerDisconnect means the player is disconnecting from the server. OnScriptLoseAccessToPlayer means the script is losing access to the player due to being unloaded.

So please have a look at that linked issue again, when considering 'fixing' this 'SA-MP bug' in open.mp. Similar thing for OnPlayerConnect btw.

WoutProvost avatar Dec 13 '23 19:12 WoutProvost

@WoutProvost same. Reported it here when it was in beta.

NexiusTailer avatar Dec 13 '23 20:12 NexiusTailer

+1

AmirShCO avatar Dec 31 '23 03:12 AmirShCO

idk this whole codebase is a mess.

deprale avatar Feb 14 '24 03:02 deprale

fix it

Hual avatar Feb 14 '24 09:02 Hual

Reverted. Not happening.

Y-Less avatar Mar 06 '24 13:03 Y-Less

Reopening, we are not finished with this.

  • If you have an argument against it you can explain it like a normal person instead of asserting dominance.
  • If you can explain why in the world anyone would want their connection related callbacks to be called in the middle of the server without anyone connecting or disconnecting, then you can be heard, if you can explain what does it exactly "fix" you can be noticed.
  • If you can explain why in the hell we can't keep it "normal" not special as if we are some stupid people making stupid behaviors, instead of just OnScriptLoadPlayer and OnScriptUnloadPlayer, then you can make a point.
  • If you can explain why are you trying so hard to push this without even a single person agreeing to it, you can be right.
  • If you explain why you think every line of code you added to fixes.inc is supposed to be justified to be added to open.mp, you can push it.
  • If you can bring at least 10 popular servers using fixes.inc (popular, not famous, the ones that actually have a good player base), we can at least consider it a good library.

Let's not forget how many things you have broken with Fixes component and I've been trying to compromise all this times. And a big reminder for anyone here reading this, just because something existed before in the community, it doesn't make it right. Even if it's from someone who's a known user and is (or used to be) respected. You have brains, do the math, if you can't even make it make sense verbally then you have a big problem to solve.

https://github.com/openmultiplayer/open.mp/issues/847

AmyrAhmady avatar Mar 06 '24 19:03 AmyrAhmady

Why does this need to be a part of the pawn component? Can't fixes extend pawn and do it there? If you don't want it don't use fixes. No clue why this needs to be in an expected part of the server.

Alasnkz avatar Mar 06 '24 19:03 Alasnkz

Wow this issue is being reopened again, i have heard this from my friend and got here. In order to solve this issue, i agree if you can just remove the abuse altogether since it have very little use-case IMHO.

And also, OnPlayerConnect is supposed to be called when player is connected to the server regardless what state the filterscript is in (being loaded or not), because the way filterscript works is you (the dev) can just load and unload it during the runtime at your own will.

That is what im going to said about this issue, +1 if this will be removed in the future.

tsssu avatar Mar 06 '24 19:03 tsssu

Again, reopening it, leaving it open for discussion to see if anyone can make sense out of it first before closing it (they can't)

AmyrAhmady avatar Mar 06 '24 20:03 AmyrAhmady

I've been trying to compromise all this times.

I would like know what you mean by this exactly. What "compromises"? All I've seen is people constantly fighting against features and trying to get them removed (like the fixes component currently being disabled by default, which it shouldn't be). It does feel like your definition of a "compromise" is "lets just half remove this feature". I even offered a compromise on this issue, i.e. that of having a flag to disable it, and that has been ignored.

Y-Less avatar Mar 26 '24 15:03 Y-Less

Just don't call it fully backwards compatible and go break shit. While fixes may or may not be a good thing if you start from scratch doesn't mean it needs to be enforced to all users especially if people recognize it as drop in replacement server. Dunno what to say you had your chance with the old macro hell of base, if it breaks shit because people don't really expect the way a fix changes behavior in their script or increases the load for a "single threaded app" then it doesn't solve shit. And I'm pretty sure there are enough people that encountered such issues due to unexpected behaviors. Can't confirm the fighting against features - users/scripters take everything making their life easier, but changing common conventions just because of the luls.. Meh.

myudev avatar Mar 26 '24 17:03 myudev

After almost 4 months of this issue being created and kept open, finally you decided to talk. And before that not only you decided to not speak out your opinion on this, but you also pushed into master to revert a change clearly not even a single thinks "makes sense", it's not about wanting anymore, it's about making sense now

I'd be fully reading your messages now but I'm done with this. I am not going to waste my time on someone who still has around 60 messages from me in different places to answer, and one is literally the one you quoted from. always ignoring the important matter and nitpicking from my wall of text.

If you have anything against this, please talk about and while you're doing that, read what others say as well. At this point I'm convinced you never read anyone's messages, not even your team mates. due to lack of any type of proper response.

And if this happens again, I guess it means you are unable to provide enough reasons for feature (not a fix in any way) you're asking for.

AmyrAhmady avatar Mar 26 '24 17:03 AmyrAhmady

Resolved

AmyrAhmady avatar Aug 19 '24 07:08 AmyrAhmady