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

Implement entity riding #330

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

Implement entity riding #330

wants to merge 22 commits into from

Conversation

NeutronicMC
Copy link
Contributor

Entities with the ability to ride other entities implement the Rider interface

Entities with the ability to be ridden implement the Rideable interface

  • Entities have a configurable number of seats, and the seats have individually configurable positions
  • When a Rider dismounts, all other current riders are reseated and a new driver is selected based on the order that they mounted the entity

Player implements the Rider interface

I've tested all of these changes

Copy link
Member

@Sandertv Sandertv left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! I've got a couple of comments. It might also be desirable to have a mount/dismount event handler in player/handler.go.

server/entity/rider.go Outdated Show resolved Hide resolved
server/session/handler_inventory_transaction.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/world/viewer.go Outdated Show resolved Hide resolved
Copy link
Member

@Sandertv Sandertv left a comment

Choose a reason for hiding this comment

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

Looking pretty good! My last comments are to add mount/dismount handlers in the player.Handler that are cancellable. Should be good to go after that if it's all been tested.

server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
Copy link
Member

@Sandertv Sandertv left a comment

Choose a reason for hiding this comment

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

Got some last comments just to clean up some of the world.Entity vs entity.Rideable. Should be good to go once you resolve those.

server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/player/player.go Outdated Show resolved Hide resolved
server/session/controllable.go Outdated Show resolved Hide resolved
server/entity/rider.go Outdated Show resolved Hide resolved
@Sandertv
Copy link
Member

So most of this PR looks good now, I just have one concern, which is Rideable.RemoveRider vs Player.DismountEntity. We expose two functions that (should) do the same, but one of them (Rideable.RemoveRider) is incomplete in the sense that it doesn't actually properly dismount the entity that is riding it. The same problem exists with Rideable.AddRider and Rider.MountEntity. I'm not personally sure how to fix this, do you have any ideas?

@NeutronicMC
Copy link
Contributor Author

I'm not sure how I would change that other than making the method names and descriptions more specific. The player needs to contain some sort of function to send packets to link entities as well as store the entity that it is mounted to. The entity needs to store all of the currently mounted players. I can't move most of the functionality into either method because it's specific to the scope of each interface. If you have any better ideas I'd be willing to try them out, but as far as I can tell this implementation works the best.

# Conflicts:
#	server/player/player.go
@JustTalDevelops
Copy link
Member

Any update on this?

@JustTalDevelops JustTalDevelops added the feature New feature or request label Jul 14, 2022
@JustTalDevelops
Copy link
Member

As Sander suggested on Discord, a good alternative is probably some sort of entity.Mount(some Entity, on Entity) function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants