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

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

Open
AmyrAhmady opened this issue Dec 12, 2023 · 29 comments
Open

Comments

@AmyrAhmady
Copy link
Member

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 AmyrAhmady pinned this issue Dec 12, 2023
@ksenonadv
Copy link
Member

+1

5 similar comments
@ReshiramZekrom1
Copy link
Contributor

+1

@pushline
Copy link

+1

@PazzOnee
Copy link

+1

@NexiusTailer
Copy link
Contributor

+1

@selvagoat
Copy link

+1

@DobbysGamertag
Copy link

+1.

Not a fan of "hacky" workarounds

@adib-yg
Copy link
Member

adib-yg commented Dec 13, 2023

+1

What about OnScriptUnloadForPlayer

@FreddieCrew
Copy link

+1.

@shierru
Copy link

shierru commented Dec 13, 2023

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 ( 👍 👎 ).
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 🥇

@Hual
Copy link
Collaborator

Hual commented Dec 13, 2023

+1 sir I need hacktoberfest shirt

@Zorono
Copy link
Contributor

Zorono commented Dec 13, 2023

+1

@Hreesang
Copy link

+1
that makes sense

@itsimperium
Copy link

+1

@Cheaterman
Copy link
Contributor

Cheaterman commented Dec 13, 2023

+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. :-)

@gHenriqueCarlos
Copy link

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

@WoutProvost
Copy link
Member

WoutProvost commented Dec 13, 2023

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:

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.

@NexiusTailer
Copy link
Contributor

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

@AmirShCO
Copy link

+1

@deprale
Copy link

deprale commented Feb 14, 2024

idk this whole codebase is a mess.

@Hual
Copy link
Collaborator

Hual commented Feb 14, 2024

fix it

@Y-Less
Copy link
Collaborator

Y-Less commented Mar 6, 2024

Reverted. Not happening.

@AmyrAhmady
Copy link
Member Author

AmyrAhmady commented Mar 6, 2024

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.

#847

@AmyrAhmady AmyrAhmady reopened this Mar 6, 2024
@Alasnkz
Copy link
Member

Alasnkz commented Mar 6, 2024

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.

@tsssu
Copy link

tsssu commented Mar 6, 2024

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.

@Hual Hual closed this as completed Mar 6, 2024
@AmyrAhmady
Copy link
Member Author

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 AmyrAhmady reopened this Mar 6, 2024
@Y-Less
Copy link
Collaborator

Y-Less commented Mar 26, 2024

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.

@myudev
Copy link
Contributor

myudev commented Mar 26, 2024

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.

@AmyrAhmady
Copy link
Member Author

AmyrAhmady commented Mar 26, 2024

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.

@openmultiplayer openmultiplayer locked as too heated and limited conversation to collaborators Mar 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests