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

Move PlayerVehicleEnter/Leave events to Player #98

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Deewarz
Copy link
Contributor

@Deewarz Deewarz commented Feb 20, 2024

Makes more sense to attach the events to the Player: he is the one who causes the events.

  • Who: the player
  • Where: In the vehicle
  • What: enter / leave

To be more precise we should attach it to Human and transfer it to Player but we don't yet have this level of layers.


This is also the case in other implementations (who created a usage).

Some examples:

@Deewarz Deewarz self-assigned this Feb 20, 2024
@Deewarz Deewarz changed the title Player vehicle enter leave on player Move PlayerVehicleEnter/Leave events to Player Feb 20, 2024
@Deewarz Deewarz force-pushed the player-vehicle-enter-leave-on-player branch from 6d7b4b9 to 4b74bc2 Compare February 20, 2024 16:42
@Deewarz Deewarz requested a review from zpl-zak February 20, 2024 16:47
@Deewarz Deewarz marked this pull request as ready for review February 20, 2024 16:47
@Segfaultd
Copy link
Member

Please put a description to the PR.

Overall comment, why renaming the events?

@Deewarz Deewarz requested a review from Segfaultd February 21, 2024 16:45
@Deewarz
Copy link
Contributor Author

Deewarz commented Feb 22, 2024

@Segfaultd PR description updated.

@Segfaultd
Copy link
Member

Thanks. Looks way better now. Open to discussion tho, what about naming something like playerEnterVehicle and playerExitVehicle. It makes more sense when it comes to litteral reading of the event

sdk.on('playerEnterVehicle', cb);

@Deewarz
Copy link
Contributor Author

Deewarz commented Feb 27, 2024

@Segfaultd I don't really know, for me it makes more sense to put "vehicle" as a prefix to the action.

It creates a sort of subcontext.

  • context: player
  • subcontext: vehicle
  • action: enter

This is the same approach I used for the setColorPrimary and setColorSecondary methods on Vehicle.

As a developer, our model of thinking is to seek context then action.
You think about putting the color, and more precisely the primary.
You think about the vehicle, and more precisely enter.

Also, it allows you to group them at a single glance when you use or document them.

I do not know if I'm clear, I don't really know how to explain it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants