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

onPlayerTeamChange Event #3370

Merged
merged 13 commits into from
May 23, 2024
Merged

onPlayerTeamChange Event #3370

merged 13 commits into from
May 23, 2024

Conversation

esmail9900
Copy link
Contributor

@esmail9900 esmail9900 commented Apr 21, 2024

implemented the onPlayerTeamChange event for the server-side. this event includes two arguments: oldTeam and newTeam. if the player doesn't have an old team, oldTeam will be false.

@tederis
Copy link
Collaborator

tederis commented Apr 21, 2024

How about enabling cancelEvent() for this new event?

@Citizen01
Copy link
Member

Citizen01 commented Apr 21, 2024

Hello @esmail9900 and thank you for your PR.

Your PR gave me some strong flashbacks from when I attempted to add this event and lost motivation.

Please view the comments on this PR #10 and check if your code complies with all that have been said in this PR.

Thank you.

Copy link
Member

@Citizen01 Citizen01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some stuff needs to be adressed.

I think the this event do not need to be cancelable right away, this can be added later with another PR as you will surely encounter some tricky problems to overcome to make it work properly.

Note: What happens if you delete the team ?
(Expectected behaviour would be that this event gets called for every players in the team and before onElementDestroy gets called so that it is already at the right place if you or someone else decides to make your event cancelable)

Added cancelEvent()
Before team getting destroyed, event will trigger for all players in the team
Copy link
Member

@Citizen01 Citizen01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks pretty good. Added some more comments.

but this need to be tested when I have time (or someone else ? be my guest)

oldTeam changed to team element instead of nil when team getting destroyed
Copy link
Member

@Citizen01 Citizen01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it locally and it works fine. Cancelling the event works too and the event is called when the team is deleted as well.

I think this PR will be ready for merge once you resolve the latest things.

Copy link
Member

@Citizen01 Citizen01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot some of my suggestions.

If they have been left on purpose, please explain why.

esmail9900 and others added 2 commits April 24, 2024 02:22
Co-authored-by: Citizen01 <jeanbaptiste.e@hotmail.fr>
@esmail9900
Copy link
Contributor Author

You forgot some of my suggestions.

If they have been left on purpose, please explain why.

@esmail9900 esmail9900 closed this Apr 23, 2024
@esmail9900
Copy link
Contributor Author

esmail9900 commented Apr 23, 2024

You forgot some of my suggestions.

If they have been left on purpose, please explain why.

mb, i just confused, im new to github :D

@esmail9900 esmail9900 reopened this Apr 23, 2024
Citizen01
Citizen01 previously approved these changes Apr 27, 2024
Copy link
Member

@Citizen01 Citizen01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay for me. Congrats on your 1st PR.

I'll ask for another review just in case. It can take a bit of time so don't worry if nothing happens in the next few days.

Thank you

Copy link
Contributor

@Nico8340 Nico8340 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@Citizen01 Citizen01 requested a review from TracerDS April 27, 2024 01:58
Moving CLuaArguments creation outside the loop and calling 'DeleteArguments()' within each iteration.
tederis
tederis previously approved these changes Apr 27, 2024
Copy link
Contributor

@TracerDS TracerDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work 🎉

Citizen01
Citizen01 previously approved these changes Apr 29, 2024
@esmail9900 esmail9900 dismissed stale reviews from Citizen01 and tederis via 734f78c May 6, 2024 23:40
@Dutchman101
Copy link
Member

There are enough passed code reviews, thanks for your contribution @esmail9900

@Dutchman101 Dutchman101 merged commit c4e18c6 into multitheftauto:master May 23, 2024
5 of 6 checks passed
MTABot pushed a commit that referenced this pull request May 23, 2024
@TheNormalnij TheNormalnij added this to the 1.6.1 milestone May 25, 2024
@TheNormalnij TheNormalnij added the enhancement New feature or request label May 25, 2024
MegadreamsBE pushed a commit to MegadreamsBE/mtasa-blue that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants