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

Add SteerVehicleEvent #3421

Closed
wants to merge 6 commits into from
Closed

Add SteerVehicleEvent #3421

wants to merge 6 commits into from

Conversation

avaruus1
Copy link
Contributor

@avaruus1 avaruus1 commented Jun 6, 2021

Sponge | SpongeAPI

Added SteerVehicleEvent. Resolves SpongePowered/SpongeAPI#2283

While having the event be cancellable would be ideal, I’m not sure how that could be done or if it’s even possible, since Mojang changed the client to handle dismounting fully client-side in 1.16.

Regardless of that, I hope this event makes it to API-8, since I believe it would be of great use when creating vehicle plugins.

@dualspiral
Copy link
Contributor

Gonna need an API PR too!

"core.RegistryAccessMixin",
"core.Vec3iMixin",
"core.dispenser.AbstractProjectileDispenseBehaviorMixin",
"data.SpongeDataHolderMixin",
Copy link
Member

Choose a reason for hiding this comment

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

Don't change the formatting here on a whim please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly sure how I managed to do that, but it should be fixed now 😅


@Inject(method = "handlePlayerInput", at = @At("HEAD"))
public void onHandlePlayerInput(ServerboundPlayerInputPacket packet, CallbackInfo ci) {
SpongeCommon.postEvent(SpongeEventFactory.createSteerVehicleEvent(
Copy link
Member

Choose a reason for hiding this comment

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

Is the event cancellable? If so, you can make the injection cancellable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pre 1.16 that would work fine, but that is not the case in newer vesions since dismounting is handled fully client-side.

@Shadow public ServerPlayer player;

@Inject(method = "handlePlayerInput", at = @At("HEAD"))
public void onHandlePlayerInput(ServerboundPlayerInputPacket packet, CallbackInfo ci) {
Copy link
Member

Choose a reason for hiding this comment

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

Make these variables final please

Suggested change
public void onHandlePlayerInput(ServerboundPlayerInputPacket packet, CallbackInfo ci) {
public void onHandlePlayerInput(final ServerboundPlayerInputPacket packet, final CallbackInfo ci) {

@Inject(method = "handlePlayerInput", at = @At("HEAD"))
public void onHandlePlayerInput(ServerboundPlayerInputPacket packet, CallbackInfo ci) {
SpongeCommon.postEvent(SpongeEventFactory.createSteerVehicleEvent(
PhaseTracker.getCauseStackManager().currentCause(),
Copy link
Member

Choose a reason for hiding this comment

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

Is it guaranteed that we'll always be on the main thread here?


@Inject(method = "setPlayerInput", at = @At("HEAD"))
public void onSetPlayerInput(final float sway, final float surge, final boolean jumping, final boolean dismount, final CallbackInfo ci) {
SpongeCommon.postEvent(SpongeEventFactory.createSteerVehicleEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to fire if the Player Input Packet is sent, regardless of whether the player is actually mounted. It would make more sense to redirect isPassenger and fire there if isPassenger would return true. You could then return false if you wanted to cancel this packet, which kind of leads me into my next question:

Pre 1.16 that would work fine, but that is not the case in newer vesions since dismounting is handled fully client-side.

...how does that make sense? The server still has to know about this and validate it. Is it because the client dismounts without validation from the server?

(I'll test it at some point, just haven't looked into it yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is going to fire if the Player Input Packet is sent, regardless of whether the player is actually mounted. It would make more sense to redirect isPassenger and fire there if isPassenger would return true.

That's probably a good idea 😅

Is it because the client dismounts without validation from the server?

That's exactly what I meant! Sorry for being unclear.

@ImMorpheus ImMorpheus added the status: needs updating hey, origin changed, you need to update label Jun 19, 2021
@@ -830,6 +831,30 @@ public void sendMessage(final net.minecraft.network.chat.Component p_241151_1_,
}
}
}

@Inject(method = "setPlayerInput", at = @At("HEAD"), cancellable = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a redirect on isPassenger, so you can avoid calling it twice, and just return false here to short circuit the if statement.

@@ -830,6 +831,30 @@ public void sendMessage(final net.minecraft.network.chat.Component p_241151_1_,
}
}
}

@Inject(method = "setPlayerInput", at = @At("HEAD"), cancellable = true)
public void onSetPlayerInput(final float sway, final float surge, final boolean jumping, final boolean dismount, final CallbackInfo ci) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private boolean impl$onSetPlayerInput(final ServerPlayer player, final float sway, final float surge, final boolean jumping, final boolean dismount)

@@ -204,6 +204,8 @@
@Shadow protected abstract boolean shadow$fireImmune();
// @formatter:on

@Shadow public abstract boolean isPassenger();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be put in the // @formatter block above, and prefixed with shadow$, so

@Shadow public abstract boolean shadow$isPassenger();

Comment on lines 173 to 175
@Shadow public float xxa;
@Shadow public float zza;
@Shadow protected boolean jumping;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put these in the // @formatter block above.


@Inject(method = "setPlayerInput", at = @At("HEAD"), cancellable = true)
public void onSetPlayerInput(final float sway, final float surge, final boolean jumping, final boolean dismount, final CallbackInfo ci) {
if(isPassenger()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefix all same class accesses with this. (but this would become this.shadow$isPassenger())

Also, space after if

public void onSetPlayerInput(final float sway, final float surge, final boolean jumping, final boolean dismount, final CallbackInfo ci) {
if(isPassenger()) {
if(sway == this.xxa && surge == this.zza && jumping == this.jumping) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This would return true; as this is a success condition - everything is the same but the action isn't being cancelled, so continue.

Comment on lines 850 to 859
Sponge.eventManager().post(event);

if(event.isCancelled()) {
ci.cancel();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could replace this with

Suggested change
Sponge.eventManager().post(event);
if(event.isCancelled()) {
ci.cancel();
}
return !Sponge.eventManager().post(event);

ci.cancel();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to return false at the end.

@ImMorpheus
Copy link
Contributor

Hey! You seem to be running an unsupported version (API 8) of SpongeAPI. Support for API 8 ended in October 2023.

API-8 is now end-of-life. We will not be issuing any more API updates for version 8 of our API1

As part of our legacy cleanup we are closing issues relating to API8/1.16


If you wish to move this forward, please rebase this PR on the current branch and reopen it.

Footnotes

  1. https://forums.spongepowered.org/t/sponge-status-update-17th-october-2023/42212#spongeapi-82-1

@ImMorpheus ImMorpheus closed this Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs updating hey, origin changed, you need to update system: event type: feature request A feature request version: 1.16 (u) API: 8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SteeringEvent
5 participants