New server side event: onPlayerTeamChange

[Citizen01]

This is my first pull-request and I added the onPlayerTeamChange server side event using the player as source and the handler arguments oldTeam, newTeam.

Btw, if the previous team was null, I'm still sending that value as is. So I don't know if it's an ok way to do.

Just got the idea because of a Scripting thread I replied with an homemade onPlayerTeamChange with a setTimer to check the players team because the guy needed that event.

source: The player who changed team
oldTeam: The previous player's team if any, nil otherwise
newTeam: The new player's team

[qaisjp]

It doesn't call if you destroy the team itself (an event should be called for every team as their new team = nil)

Whilst you're there, please try and merge those three commits into one and force push :)

You can change the tab settings in the visual studio settings>editor section

Good job.

[qaisjp]

In addition, could this be cancellable? I'm not sure how this should behave if you try to set the player team inside of the event (even without cancelling).

You don't have to do this now though, this is an idea for expansion in the future.

[Citizen01]

Oh yeah didn't think about that case.
I will modify it and rewrite the git log so it will be only 1 commit.
This is kinda the first time I really get into the source code of MTA but I'll check other events to see how one event can be canceled (for another pull request ofc).
Thanks.

[Citizen01]

Alright, I finally managed to fix the bugs I had when I tried to add the support of the cancelEvent feature but now everything is working. I did a lot of tests and everything work as expected.
I'm asking for a code review for validation. I don't know if the modifications I had to do in the CElementDeleter::Delete is okay but if it is not, I'll need some help on that one.

[qaisjp]

Hey, are said updates in that commit from two months ago?

[Citizen01]

Some of the modifications are yeah. But then I stopped because i was facing a problem and I was busy with school projects. Last night I finally fixed the problem, made a commit and merged it. On my local git log the date was correct but apparently it is not on github. There are 5 files now in this commit instead of 3 from the previous one.

[qaisjp]

Okay, that date issue isn't a problem. I'll review it soon

[qaisjp]

The "previous" team returns a userdata value pointing to a destroy team, which it should do. Good call.

Okay, things appear to work perfectly fine except for two tiny things:

1. team sets inside the event don't work. (Infinite loops cause a stack overflow as expected and behave correctly).
2. When you leave the server, the team event isn't called (prev=prev, current=nil) - @ccw808 can weigh in on whether this is needed.

Once these issues are sorted out I think the feature is done (assuming ccw is fine with the rest of the code, since he hasn't commented on anything else).

Reproducing the issues

See this code (I explain below it):

addEventHandler("onPlayerTeamChange", root,
    function(previous, current)
        local text = source.name .. "'s' team changed from " .. (isElement(previous) and previous.name or "none") .. " to " .. (current and current.name or "none")
        outputServerLog(text)
        outputChatBox(text)

        if current==bob then
            source.team = changed
        end
    end
)

addEventHandler("onResourceStart", resourceRoot, function()
    changed = Team("No bob!")
    bob = Team("Bob")
    Team("Bill")
end)

It creates three teams: Bob, Bill, and No bob!.

This is what I do, and what happens:

• Set my team to Bill
• Outputs qaisjp's' team changed from none to Bill ✔
• Scoreboard shows I am in Bill, ✔
• Set my team to Bob
• Outputs qaisjp's' team changed from Bill to Bob, ✔
• Scoreboard shows I am in Bob, ✔
• onTeamChange sets my team to "No bob!", ✔
• Outputs qaisjp's' team changed from Bill to No bob! ✔
• Scoreboard still shows I am in Bob, ✘
• qais.team.name outputs "Bob" ✘

To reproduce the second issue you can just disconnect from the server, the server log doesn't display anything. On the whole, good job!

[qaisjp]

I should also mention that canceling didn't work (replacing source.team = changed with cancelEvent()) - I am not sure whether you finished implementing that or not (if you didn't finish that off, no worries).

[Citizen01]

For the first issue, I see why it is not working but I guess it should work if you call cancelEvent() and then change the team inside the event handler. I will fix it ayway so it's not needed to call it.

The cancelEvent() has been done but a mistake in my if statement make it only working if you already have a team (and so that you are switching for another one).

I need to rewrite it to fix this issue and make what ccw pointed out about m_bIsBeingDeleted

[qaisjp]

Hey @Citizen01, is there any progress on this? What do you think about this issue being closed, since according to your last comment the code needs to be rewritten?

[Citizen01]

Funny thing I was getting back into it tonight as I hate when things are not done.
Yeah I need to rewrite it, but I don't think you should close it as I will create a duplicate because a closed Pull Request will stay in the "history" of the repo. I mean we will see that PR twice in the logs.
If it doesn't matter then you can close it of course.
Will get back to you guys by the end of tomorrow or monday if I'm struggling with the element deleter : >

[qaisjp]

Okay, I'll just squash-merge this PR when it's completed instead.

[qaisjp]

I'm going to close this PR for now. Once it has been completed it can be reopened. Let us know if you need anything @Citizen01.