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 support for rotation argument handling. #12090

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

NonSwag
Copy link
Contributor

@NonSwag NonSwag commented Feb 9, 2025

Resolves #11395

Introduced a RotationResolver to handle rotation arguments and added related methods to ArgumentTypes, and argument provider classes. This enables resolving and processing rotation data using CommandSourceStack.

The Vector2f in RotationArgument was just a temp solution because I wasn't sure on whether to add a rotation object that clearly combines a yaw and pitch

Introduced a `RotationResolver` to handle rotation arguments and added related methods to `ArgumentTypes`, and argument provider classes. This enables resolving and processing rotation data using `CommandSourceStack`.
@NonSwag NonSwag requested a review from a team as a code owner February 9, 2025 14:54
@NonSwag
Copy link
Contributor Author

NonSwag commented Feb 9, 2025

image

public class TestCommand {
        public static LiteralCommandNode<CommandSourceStack> create() {
            return Commands.literal("teleport")
                .requires (commandSourceStack -> commandSourceStack.getSender() instanceof Player)
                .then(Commands.argument("position", ArgumentTypes.finePosition())
                    .then(Commands.argument("rotation", ArgumentTypes.rotation())
                        .executes (ctx -> {
                            var sender = (Player) ctx.getSource().getSender();
                            var position = ctx.getArgument("position", FinePositionResolver.class).resolve(ctx.getSource());
                            var rotation = ctx.getArgument("rotation", RotationResolver.class).resolve(ctx.getSource());
                            sender.teleportAsync(position.toLocation(sender.getWorld()).setRotation(rotation.y, rotation.x));
                            return Command.SINGLE_SUCCESS;
                        })))
                .build();
        }
    }

@NonSwag NonSwag closed this Feb 9, 2025
@NonSwag NonSwag reopened this Feb 9, 2025
Copy link
Member

@Warriorrrr Warriorrrr left a comment

Choose a reason for hiding this comment

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

I think a new rotation wrapper class would be good for this, but will want see what others think first.

@lynxplay
Copy link
Contributor

I'd also be in favour of a rotation type, mostly because we can add some utils on there to e.g. convert to a direction vector.
x and y are also just terrible names so, another good reason for a quick interface + package private record impl.

NonSwag and others added 5 commits February 11, 2025 15:32
Introduce the `Rotation` interface and its `RotationImpl` implementation to represent orientation with yaw and pitch. This provides a foundation for handling rotational data across the API.
Introduce `addRotation`, `subtractRotation`, and `setRotation` methods to allow manipulation of `Rotation` objects directly within the `Location` class. These methods enhance code clarity and reduce repetitive manual handling of yaw and pitch adjustments.
Replaced `Vector2f` with the new `Rotation` API in relevant methods and interfaces. This improves type clarity and aligns with the updated math utility framework. Changes include method signatures and implementations for better consistency.
Added JavaDoc descriptions for the `pitch()` and `yaw()` methods, as well as the interface itself. This improves code readability and provides clear documentation for developers.
@NonSwag
Copy link
Contributor Author

NonSwag commented Feb 11, 2025

... we can add some utils on there to e.g. convert to a direction vector.

My math skills are too... non-existent... to do something like that
as far as I know a vector consists out of 3 components_?_

@lynxplay
Copy link
Contributor

Just ignore for now, we don't need to add utils yet.
Was just an argument for using a proper type instead of reusing joml's Vector2F

@NonSwag NonSwag requested a review from Warriorrrr February 11, 2025 15:48
* Represents a rotation with specified pitch and yaw values.
*/
@NullMarked
public interface Rotation {
Copy link
Member

Choose a reason for hiding this comment

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

There should be some way to obtain an instance of this if we want it to be useful outside of commands, RotationImpl can be moved into the math package in api and a static method can be added in this class, the same is currently done in Position for example.

package io.papermc.paper.util;

import io.papermc.paper.math.Rotation;
import org.bukkit.util.Vector;
Copy link
Member

Choose a reason for hiding this comment

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

leftover unused import here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Awaiting review
Development

Successfully merging this pull request may close these issues.

RotationArgument missing
3 participants