-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Expose certain scoreboard related argument types #12541
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I'll poke some other team members about this, so I'd wait a bit with implementing my feedback given how drastic it is.
paper-api/src/main/java/io/papermc/paper/InternalAPIBridge.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/InternalAPIBridge.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/InternalAPIBridge.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/command/brigadier/argument/ArgumentTypes.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/command/brigadier/argument/ArgumentTypes.java
Outdated
Show resolved
Hide resolved
* @throws IllegalArgumentException if entity is null | ||
* @see #getScores(ScoreHolder) | ||
*/ | ||
@NotNull Set<Score> getScoresFor(@NotNull ScoreHolder holder) throws IllegalArgumentException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods are just copies from the ones above?
What is the point of these?
paper-server/src/main/java/io/papermc/paper/command/brigadier/ApiMirrorRootNode.java
Outdated
Show resolved
Hide resolved
* @param targetHolder The target of the operation | ||
* @param sourceHolder The source of the operation | ||
*/ | ||
void apply(Scoreboard scoreboard, Objective targetObjective, Objective sourceObjective, ScoreHolder targetHolder, ScoreHolder sourceHolder) throws CommandSyntaxException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I am not much of a fan of this split between SimpleOperation and Operation internally.
apply(int, int) is only defined on simple operations. This makes it seem like it is universally applicable (which it isn't, CommandSyntaxException is thrown for the swap operation).
Having to pass all this BS just to construct a ScoreAccess also seems overkill.
Plugins will not be using scoreboards to store/modify their data.
I think we'd be better off on implementing ScoreAccess in our own type with a noop display name / override.
That way we can a) have apply(int, int) be always functional and simply return the "target", or "left" side here.
We could also expose an apply(int,int)Int2IntPair that yields both for people caring about swap.
This scoreboard/objective based method seems just very very useless/cumbersome to work with.
import org.jspecify.annotations.Nullable; | ||
|
||
@NullMarked | ||
public class OperationImpl implements Operation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the downsides mentioned in the API type apply here but review of this without changes to the API type is pretty useless.
import org.jspecify.annotations.NullMarked; | ||
|
||
@NullMarked | ||
public class CraftScoreHolder implements ScoreHolder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given I believe this requires a restructure, review of this isn't needed.
9838985
to
0b2ca4b
Compare
This commit is a squash and fixup commit of multiple previous commits. Fixed the JavaDocs and formatting.
0b2ca4b
to
4e0ae70
Compare
Closes #12540
This PR exposes a few scoreboard related Vanilla arguments for use in-API.
OperationArgument
ScoreHolderArgument
There is no reason to expose the Vanilla one, as that one is just a wrapper around a string argument.ObjectiveArgument
Operation Argument
Example usage: Implementing a simple /compute command.
Preview:Code:
ScoreHolder Argument
Example usage
Preview:Code: