Add option to disable connection events on filterscript load/unload

[Andosius]

Added a possibility to enable legacy behavior. Writing new callbacks seems overkill for me (people discussed that in the linked issue).

Option is called "game.use_filterscript_load_events". Naming can definitely be discussed.
Default is true, false for legacy behavior with no OnPlayerConnect/OnPlayerDisconnect calls when loading/unloading.

That could be an easy solution for #807 which defaults to new logic.

[NexiusTailer]

I'd just move this behaviour change into fixes component so it could be easily disabled as it already has a bunch of the other breaking stuff. It is more convenient to throw it all away at once if it's all in one place.

[Andosius]

I don‘t think moving that part to the Fixes component solves the issue.

What if people use filterscripts which send messages when a player disconnects? Unloading the script to activate a new version would flood the chat and that can‘t be the intended behavior.
People can‘t know that would happen - the current implementation is counter intuitive.

Why not let people choose the behavior themselves by simply changing true to false inside their config file?

[NexiusTailer]

Moreover it's in the current discussion here.
My opinion was always about it shouldn't be even enabled. But since many questionable fixes stuff were brought without much consideration of how it affect current samp scripts, it's in omp server now and it's enabled. Not sure it will be reverted in near future, so, the easiest and most realistic way is to move all such trash in fixes component and then just disable it on your own, all at once without further pain with other intrusive things. Adding the config variable for each one and keeping it enabled by default keeps the problem by default for every person until he cleary understand what caused his problem. It will take some time for him to understand that the next compatibility problem he find will most likely again caused by something from fixes component, so, in the end, sooner or later, disabling it all (by disabling fixes component) will solve everything he's struggling with.

[AmyrAhmady]

There will be no toggle, no implementing in another component, or anything. The problem with this was not making sense due to the callback names making absolutely zero sense and conflicts with people's scripts.
The implementation will be completely removed, and replaced with with a new one, new callbacks that get called on script initialization

Thanks for your PR but it doesn't suit what our goals are, so I'm closing this.

[Y-Less]

Since people still don't seem to be aware of this; open.mp is a direct continuation and successor of fixes.inc, fixes.inc 2.0 as it were. Complaints of "I like open.mp but not fixes.inc" make no sense, they're one and the same. We already had a buggy version of the server, it's called the SA:MP server, and if you like the bugs that's the version to use.

However, I'm fine with options to disable things, for those who don't want to fix things, but that's usually for experienced coders with established scripts they don't/won't/can't update (the only ones who actually complain about having to update rather than just getting on with it and updating). New scripts and newbie scripters are the ones who need consistent behaviour, such as OnPlayerConnect behaving the same in main scripts and side scripts. If you want inconsistent/legacy/broken/etc behaviour that's fine, but something you must opt in to. The other option is to make these behaviours the default and require new coders to opt out of them, which is clearly wrong. The burden of advanced configuration must fall on those with the experience to do so, not those brand new who just want a simple experience straight out of the box. If your ancient script already works around old issues and you don't want to adjust that stable code you had to struggle to develop in the first place, congratulations, but we aren't going to inflict the same struggles by default on others just for the sake of "BuT I dOn'T wAnT tO uPgRaDe".

[Andosius]

I don't know what came to your mind to write such a toxic reponse. Amir closed that pull request 5 days ago - what do you think that message will achieve?
Not even talking about the fact I haven't played SAMP for the last 4 years and don't profit with that PR by any means.

You may not have seen it, but the default behavior would've been the current, already implemented behavior.
The pull request had one simple reason: To give people the option to opt in for old behavior to keep compatibility.

There is no reason to react overly emotional over a pull request. It got closed, Amir explained why and that's totally fine.
I don't see the need for aggression here.

[Southclaws]

"experienced coders with established scripts" is 99% of the remaining audience.

[Y-Less]

I don't know what came to your mind to write such a toxic reponse. Amir closed that pull request 5 days ago - what do you think that message will achieve? Not even talking about the fact I haven't played SAMP for the last 4 years and don't profit with that PR by any means.

You may not have seen it, but the default behavior would've been the current, already implemented behavior. The pull request had one simple reason: To give people the option to opt in for old behavior to keep compatibility.

There is no reason to react overly emotional over a pull request. It got closed, Amir explained why and that's totally fine. I don't see the need for aggression here.

I have re-read the response, and I am not sure which part you think is so aggressive. Maybe the end showed a little more frustration at the fact that I've had to explain this to some people dozens of times without them ever learning, but I did not intend any of the level of toxicity that you seem to have inferred. Sorry.

As to what I hoped to achieve, I was just trying to provide the facts of why some things are the way they are; give some context that I can point others to in the future. The point was not to target this particular PR in any way, it just happened to be a convenient location to explain some things in a static location (rather than Discord, where the explanation has been lost countless times). Maybe a blog post would be better.

[NexiusTailer]

The clear and transparent poll is the best place to refer and point to in the future.

---

Complaints of "I like open.mp but not fixes.inc" make no sense, they're one and the same

Where did the BypassDialog fix go if this is the case? The same with a bunch of unimplemented YSF natives. You see, it's doesn't work like that so it makes no sense to claim it's a full successor / they're the same, at the same time porting only some parts of the stuff from these two libs (not even used by the most of server developers) in omp server which kinda intend to replace the whole samp server for the whole community.

New scripts and newbie scripters are the ones who need consistent behaviour, such as OnPlayerConnect behaving the same in main scripts and side scripts.

"Consistent" is when it is not fake called when players are already connected, only and exactly this way: player connected -> it is called, player did not -> it is not.

[Southclaws]

My vote: disagree and commit to a solution the community is asking for.

[Y-Less]

"Consistent" is when it is not fake called when players are already connected, only and exactly this way: player connected -> it is called, player did not -> it is not.

That's a perfectly reasonable argument. If they are already on the server, OnPlayerConnect should not be called when a script starts. This is clear, logical, consistent, and not what SA:MP does either. The behaviour before was: player connected -> it is called, player did not -> depends on the script type. So to make it consistent we can either disable the call in gamemodes, or add it to filterscripts. We opted for the latter.

It is also worth considering this from a library writer's point of view. You have some per-player initialisation code run when a player connects. Let's say you move all that code in to a generic function called Lib_InitialiseForPlayer(playerid), you now have three options:

1. OnPlayerConnect is never called for existing players (player did not -> it is not behaviour). You need to hook both OnFilterScriptInit and OnGameModeInit and loop through all players there, if IsPlayerConnected call Lib_InitialiseForPlayer.
2. OnPlayerConnect is sometimes called for existing players (SA:MP behaviour). You now only need to hook OnFilterScriptInit and have a single loop, because OnPlayerConnect is called for existing players when the gamemode starts, but that's still some superfluous overhead that shouldn't be needed. Plus there's now additional knowledge required as to when you should initialise things and when you shouldn't (I hope you tested your include in both places).
3. OnPlayerConnect is always called for existing players (fixes.inc/open.mp behaviour). No loops needed. No script-type-specific knowledge needed. Your code in OnPlayerConnect always works to set up data for players, rather than sometimes works for some players in some scripts.

If anyone has an actual technical reason as to why either of the first two behaviours are better for coders - simpler, more logical, less cognitive overhead, DRY, etc; I would love to hear them. Rather than just "it changed", which, yes, yes it did.

As I said, I'd actually be fine with this PR. If for some reason you can't edit your filterscripts to remove per-player initialisation code on script init, then you can disable the feature, but I also don't believe that this entire argument is over more than two filterscripts in the world where that is the case.

[NexiusTailer]

The behaviour before was: player connected -> it is called, player did not -> depends on the script type.

Rather depends on was the server restarted from gmx or not. When it was restarted without truing on/off, all connected clients received RPC GameModeRestart (40), fully simulating a reconnect on a clientside (clearing pools of everything created before, etc - so it really looks like reconnection) because it's reasonable when you changed gamemode/restarted the server that way. What we have when you decided to spoof the public calling every time it loads or unloads any filterscript? Right, mostly problems, because filterscripts are not gamemode and restarting the whole server (via gmx) not the same as (un)load random fs at runtime.

It is also worth considering this from a library writer's point of view.

Exactly, it is worth considering from a scripter's point of view after this change. If he always did a fairly intuitive action in the form of a loop for all clients inside OnFilterScript(Init/Exit), doing something for already existing players if the filterscript was (un)loaded at runtime, and also intuitive per-player action for a really newly (dis)connected player, when the fs is already executing, inside OnPlayer(Connect/Disconnect), that's what he has now:

• His code will be double-applied because of executing both a loop in OnFilterScript(Init/Exit) and a per-player action in OnPlayer(Dis)connect. You may say "change it" as usual, but is it so intuitive now? Does it become better than it was? Completely rhetorical question, ofc not.
• He now cannot determine if a player was actually (dis)connected to the server at the time of the filterscript (un)loaded or not. Developers usually do some special (different) actions for each of these two cases. For example: when the filterscript loaded at runtime and there are already connected players, there is no sense to send anything like "Hey! Welcome to the server, Mike". There wasn't gamemode restart (gmx nor a real one), they can already be spawned and do any actions for a long time up to the moment when the loaded filterscript mislead you (as a developer) and them (as players) with similar behavior. But sending wrong/misleading chat message is just a quick example. More or less similar could be with disconnection event which can lead to much more significant artifacts if the scripter on the filterscript side actually thought that this player was no longer on the server catching spoofed OnPlayerDisconnect and do something which ruin his gameplay when that fs be unloaded.

And in fact, these arguments are on the surface, you just need to discuss any such things before you introduce them without asking anyone, not after, when they are already in and people need to make the whole topics and polls thinking how they can revert this yet another poorly thought out change in behavior, which previously suited everyone and there was no request from anyone to change it at the default level.

[Andosius]

From my point of view, OnPlayerConnect and OnPlayerDisconnect should only be called inside a GM/FS when the player actually connects/disconnects.
Calling these events on OnFilterScriptInit is a huge logical conflict. OnFilterScriptInit does not imply player events.

But changing that would come with the consequences you described: FilterScripts have to handle player initialization by looping through all players. Personally I see these options:

1. Add two new callbacks for (de-)initialization matters, even if only the minority of the community may use them
2. Add a new function (something alike InitializePlayersCall("<function_name>")) which can be called inside OnFilterScriptInit and calls the function with playerid as parameter.

Option number two prevents the implementation of new callbacks which have to be called everytime a FilterScript gets (un)loaded and you don't have to implement the function in your codebase.