Add SteerVehicleEvent #2364

avaruus1 · 2021-06-06T21:16:33Z

Sponge | SpongeAPI

src/main/java/org/spongepowered/api/event/entity/living/player/SteerVehicleEvent.java

gabizou · 2021-06-07T00:32:42Z

src/main/java/org/spongepowered/api/event/entity/living/player/SteerVehicleEvent.java

+    /**
+     * The possible scenarios of steering left or right.
+     */
+    enum Sway {


I understand it's effectively either left, right, or none, but I'm partial to still not having any enums here.

I’d argue that using enums is the best solution here, since it’s more user friendly than the alternatives (I can think of). Additionally, they allow you to use switch statements, which is quite nice.

The alternatives that I can think of are
a) Just providing the float values straight from the packet
b) Having 4 distinct booleans (left, right, forwards, backwards)

The problem I have with alternative a is that it’s very easy to confuse the two directions. Does a positive number mean left or right?

Alternative b is also problematic in terms of clarity. Having distinct booleans for pressing left and right may give someone the impression that left and right may be true simultaneously, if both a and d are pressed.

Of course, both of these problems can be solved with good javadoc comments. However I personally still prefer enums, because I find them more user friendly and I like switch statements :P

The bigger problem I have here is that you're bleeding implementation details into the API.

Take this on its own, why is "RIGHT" or "LEFT" -1 and 1? Why does a plugin developer care? Quite simply, they don't and they shouldn't. If Mojang change the packet, we have to change the API in a breaking way. This is why we have our pseudo enums that hide as much implementation as possible.

If we are to have enums, they should go in the util package and be named in a neutral way so they can be reused.

That said, thinking beyond this, it's quite clear that the packets can return sways and surges between -1 and 1, not just 1, 0 and -1 - is this information useful? For sway, is it simply an angular velocity (so, should we expose degrees per unit)? For surge, is it a linear velocity?

dualspiral

We have RideEntityEvent. While this seems to be specific to Vehicles, should this event really be a sub-event of that?

Dismounting already has its own sub-event.

dualspiral · 2021-06-09T19:44:37Z

src/main/java/org/spongepowered/api/event/entity/living/player/SteerVehicleEvent.java

+import org.spongepowered.api.util.annotation.eventgen.GenerateFactoryMethod;
+
+/**
+ * Called each tick for any {@link ServerPlayer player} riding a vehicle.


Is this sensible? Having 50 players in boats riding about is going to get very spammy very quickly.

Why might you want to check this every tick? Would it be better to detect when the steering changes?

dualspiral · 2021-06-09T19:59:28Z

src/main/java/org/spongepowered/api/event/entity/living/player/SteerVehicleEvent.java

+     *
+     * @return The player
+     */
+    ServerPlayer player();


If this is an event concerning a vehicle, then the Vehicle should be exposed here.

I believe this is still a problem, what vehicle are we steering?

Also, more than likely, the steering is being caused by a player, which will likely be "required" as part of the Cause, but you've omitted the vehicle, my guess because it's accessible by the Player.vehicle() method?

dualspiral · 2021-06-09T19:59:57Z

src/main/java/org/spongepowered/api/event/entity/living/player/SteerVehicleEvent.java

+     *
+     * @return Whether the player is attempting to dismount the entity.
+     */
+    boolean unmount();


Dismounting should be a different event entirely to steering.

dualspiral · 2021-06-09T20:09:48Z

src/main/java/org/spongepowered/api/event/entity/living/player/SteerVehicleEvent.java

+    /**
+     * The possible scenarios of steering left or right.
+     */
+    enum Sway {


The bigger problem I have here is that you're bleeding implementation details into the API.

Take this on its own, why is "RIGHT" or "LEFT" -1 and 1? Why does a plugin developer care? Quite simply, they don't and they shouldn't. If Mojang change the packet, we have to change the API in a breaking way. This is why we have our pseudo enums that hide as much implementation as possible.

If we are to have enums, they should go in the util package and be named in a neutral way so they can be reused.

That said, thinking beyond this, it's quite clear that the packets can return sways and surges between -1 and 1, not just 1, 0 and -1 - is this information useful? For sway, is it simply an angular velocity (so, should we expose degrees per unit)? For surge, is it a linear velocity?

avaruus1 · 2021-06-26T23:54:12Z

We have RideEntityEvent. While this seems to be specific to Vehicles, should this event really be a sub-event of that?

Quoting the Javadoc, RideEntityEvent is "an event that involves an entity riding another." While this is fine for mount & dismount, only a player can steer a vehicle.

dualspiral

I'm concerned about the floats. I get why you did this, but it's not really what I meant.

This event is concerned with acceleration. When you steer a vehicle, you are accelerating (or decelerating) it in a given direction. My point was that you could work out the acceleration of the vehicle, However, that might be quite hard... maybe just a direction or some pseudo class as mentioned for "sway"?

I'm guessing the vehicle has a current velocity? If you stop steering it, it doesn't come to complete stop immediately. Thus, would it be useful to have the velocity, or at least the direction of travel in this event, so there is some easy way to get which direction "left" and "right" really are?

I still think this needs to be cancellable. We've already removed the unmounting part of it, so cancelling it should be trivial now, right?

dualspiral · 2021-08-08T19:52:35Z

src/main/java/org/spongepowered/api/event/entity/living/player/SteerVehicleEvent.java

+     *
+     * @return The sway
+     */
+    float sway();


I know that you did this based on my comments, but it's not really what I meant. I talked about the acceleration in given directions.

Let's take this back a step for a moment. What information are we trying to convey to plugins - I'm probably better asking why is this is useful event? What does this do that the move event doesn't, for example? Why would preventing a steer be important?

Again, I know I talked about why we might not want this as an enum, but making this a float doesn't solve the problem itself. You're exposing numbers from the packet which means nothing. An enum was better than this... and I'm honestly thinking that we should just have a Steer psuedo-enum that can take in a direction and return a rotated direction.

In fact, we could possibly use something like the Rotation class here, you're either steering clockwise (to the right) or anti-clockwise (to the left), and we can include some useful functions on the class to aid with transformation of direction.

dualspiral · 2021-08-08T19:52:55Z

src/main/java/org/spongepowered/api/event/entity/living/player/SteerVehicleEvent.java

+     *
+     * @return The surge
+     */
+    float surge();


Kind of the same problem as "sway"

dualspiral · 2021-08-08T19:53:32Z

src/main/java/org/spongepowered/api/event/entity/living/player/SteerVehicleEvent.java

+     *
+     * @return Whether the player is attempting to jump.
+     */
+    boolean jump();


Should jump really be part of the steer event? I'd argue not - because it performs the same function as jumping out of a vehicle, this is covered by the move event.

aromaa · 2021-12-01T17:24:45Z

Taking over this.

Changed the API to be more friendly to consume. This is at least somewhat how I would imagine the values would be exposed.

The input can be directly used to manipulate the entitys velocity.

aromaa · 2021-12-01T17:30:25Z

Also to add; This event is completly based on input. It does not take into account the current velocity, movement or direction.

If the plugin developer wants to make the entity controllable by the rider, it should use the riders direction and then add velocity to the entity based on the input.

dualspiral · 2021-12-28T13:55:21Z

I really don't know what I think about this - other than it's become a victim of ideas rather than solving a specific problem.

What is the problem we want to solve? To be able to stop someone from steering clockwise/counter-clockwise? Or is there some more fine grained control we need? Once we know what we want to do, we can design the event around that.

aromaa · 2021-12-28T14:16:00Z

We are trying to solve how can we allow plugin to take control of how vehicle is being moved based on controls given by the player. To better visualize this, lets take a look at a real world example; Hypixel Turbo Kart Racers.

The "vehicle" the player is inside is actually a armor stand with item on its head. The item has 3D model to make it look like you are sitting inside actual race car. To be able to "control" this vehicle, the client sends the steering packet, which normally would do nothing. In this case however, the vanilla action is overridden by a plugin to give the armor stand velocity based where the player would like to move (wasd). When the player tries to jump (pressed space), a honk is used instead and when the player tries to sneak (pressed shift), the car starts to drift. To make the visuals look more pleasing, it also moves the armor stands head around to allow the car to tilt a bit when making a turn.

To solve that, we need a way for the plugin to interpret the steering packet and implement its own logic to override the vanilla behavior.

dualspiral · 2021-12-28T14:27:10Z

we need a way for the plugin to interpret the steering packet

To clear any confusion, an event should not be a façade for a packet itself, which is what this event tried to become. Instead, I see three significant events that should occur here:

Steer - left and right
Jump - this could be part of "steering", but I feel like it's a separate action
Dismount - we already have this

In all cases you're riding an entity. Should we then be including this in RideEntityEvent as sub events? I'd propose we probably would have:

RideEntityEvent.Steer: with just a left and right (I know what I previously said, I am a victim of my own ideas too) - if we need to flesh it out later, so be it.
RideEntityEvent.Jump: hopefully this one is obvious
RideEntityEvent.Dismount: if we have to tell the server to remount if this is cancelled, so be it

Does that cover what you're interested in doing?

@gabizou @Zidane I suppose a good question here is if steer and jump should be sub-events or RideEntityEvent or not - any opinions?

aromaa · 2021-12-28T14:43:00Z

I did consider to implement them all as sub events (as I did for dismount) but then got a bit worried about whatever that would be too impactful for performance reasons. It looks way cleaner in my opinion and would 👍 for it. However, what data type should we use when exposing the steering direction? Booleans? New enum? Existing enum? Vector like I used?

Also makes sense to move this all to RideEntityEvent, lets do that.

gabizou · 2022-01-08T08:33:05Z

I suppose a good question here is if steer and jump should be sub-events or RideEntityEvent or not - any opinions?

I have no personal opinions here, sub events mean more to filter out. I don't know how spammy these events would be otherwise.

However, what data type should we use when exposing the steering direction? Booleans? New enum? Existing enum? Vector like I used?

Correct me if I'm wrong, but there's 8 possible directions, (forward, back, left, right, forward left, forward right, back right, back left) at which point I personally believe we should just expose the Vector3d direction aligned by the entity currently riding (maybe even expose the Vector3d of the direction from the entity riding vs the direction aligned to the world's coordinates).

dualspiral · 2022-01-08T10:40:20Z

Correct me if I'm wrong

Steering is clockwise or anti-clockwise - it's relative to the steering entity, not the world. Thus, that or left and right will suffice.

ImMorpheus · 2023-11-03T20:20:10Z

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 API¹

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.

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

avaruus1 mentioned this pull request Jun 6, 2021

Add SteerVehicleEvent SpongePowered/Sponge#3421

Closed

gabizou reviewed Jun 7, 2021

View reviewed changes

src/main/java/org/spongepowered/api/event/entity/living/player/SteerVehicleEvent.java Outdated Show resolved Hide resolved

gabizou reviewed Jun 7, 2021

View reviewed changes

dualspiral reviewed Jun 9, 2021

View reviewed changes

ImMorpheus added api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023) system: event type: feature request A feature request labels Jun 12, 2021

avaruus1 requested a review from dualspiral June 28, 2021 16:29

dualspiral reviewed Aug 8, 2021

View reviewed changes

avaruus1 and others added 6 commits December 1, 2021 08:25

Add SteerVehicleEvent

2dbec14

SteerVehicleEvent improve spelling in javadoc comments

fa79d0c

Improve SteerVehicleEvent

a532560

Made SteerVehicleEvent cancellable

5912277

Fix Javadoc comment in SteerVehicleEvent

85322b9

Take two

1a77bae

aromaa force-pushed the api-8 branch from 12b6d00 to 8727c7a Compare December 1, 2021 17:07

ImMorpheus closed this Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SteerVehicleEvent #2364

Add SteerVehicleEvent #2364

avaruus1 commented Jun 6, 2021

gabizou Jun 7, 2021

avaruus1 Jun 8, 2021

dualspiral Jun 9, 2021 •

edited

Loading

dualspiral left a comment

dualspiral Jun 9, 2021

dualspiral Jun 9, 2021

gabizou Aug 4, 2021

gabizou Aug 4, 2021

dualspiral Jun 9, 2021

dualspiral Jun 9, 2021 •

edited

Loading

avaruus1 commented Jun 26, 2021

dualspiral left a comment

dualspiral Aug 8, 2021

dualspiral Aug 8, 2021

dualspiral Aug 8, 2021

aromaa commented Dec 1, 2021

aromaa commented Dec 1, 2021

dualspiral commented Dec 28, 2021

aromaa commented Dec 28, 2021

dualspiral commented Dec 28, 2021 •

edited

Loading

aromaa commented Dec 28, 2021

gabizou commented Jan 8, 2022

dualspiral commented Jan 8, 2022

ImMorpheus commented Nov 3, 2023

Add SteerVehicleEvent #2364

Add SteerVehicleEvent #2364

Conversation

avaruus1 commented Jun 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dualspiral Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

dualspiral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dualspiral Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

avaruus1 commented Jun 26, 2021

dualspiral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aromaa commented Dec 1, 2021

aromaa commented Dec 1, 2021

dualspiral commented Dec 28, 2021

aromaa commented Dec 28, 2021

dualspiral commented Dec 28, 2021 • edited Loading

aromaa commented Dec 28, 2021

gabizou commented Jan 8, 2022

dualspiral commented Jan 8, 2022

ImMorpheus commented Nov 3, 2023

Footnotes

dualspiral Jun 9, 2021 •

edited

Loading

dualspiral Jun 9, 2021 •

edited

Loading

dualspiral commented Dec 28, 2021 •

edited

Loading