-
-
Notifications
You must be signed in to change notification settings - Fork 376
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 ViaVersion Support #6616
base: dev/feature
Are you sure you want to change the base?
Add ViaVersion Support #6616
Conversation
My opinion is that ExprPlayerProtocol version should be changed to an event-value in the server list ping event. That's the only time the version number ever differs from the server's, and hence the only time it's ever useful. |
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.
I don't like building around other plugins in principle, but in this case I'm not particularly against the change since it's a pretty good use case.
src/main/java/ch/njol/skript/expressions/ExprPlayerProtocolVersion.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprPlayerProtocolVersion.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprPlayerProtocolVersion.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprPlayerProtocolVersion.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprPlayerProtocolVersion.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprPlayerProtocolVersion.java
Outdated
Show resolved
Hide resolved
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.
Good it looks better now
Request for more reviews/opinions |
src/main/java/ch/njol/skript/expressions/ExprPlayerProtocolVersion.java
Outdated
Show resolved
Hide resolved
if (Skript.classExists("com.destroystokyo.paper.network.NetworkClient") || viaVersionExists) { | ||
register(ExprPlayerProtocolVersion.class, Integer.class, "protocol version", "players"); | ||
} |
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.
if (Skript.classExists("com.destroystokyo.paper.network.NetworkClient") || viaVersionExists) { | |
register(ExprPlayerProtocolVersion.class, Integer.class, "protocol version", "players"); | |
} | |
if (Skript.classExists("com.destroystokyo.paper.network.NetworkClient") || viaVersionExists) | |
register(ExprPlayerProtocolVersion.class, Integer.class, "protocol version", "players"); |
src/main/java/ch/njol/skript/expressions/ExprPlayerProtocolVersion.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprPlayerProtocolVersion.java
Outdated
Show resolved
Hide resolved
…sion.java Co-authored-by: cheeezburga <[email protected]>
…sion.java Co-authored-by: cheeezburga <[email protected]>
…sion.java Co-authored-by: cheeezburga <[email protected]>
Description
This PR adds ViaVersion support to ExprPlayerProtocolVersion. If ViaVersion is not installed on the server it defaults to the current one.
Why?
The current expression only returns the server's version, not the player's.
To sum up the discussion on discord and my opinions
It was recommended to use reflection as it was deemed unnecessary to have another dependency while compiling the jar. While this goes against the code conventions of Skript as it may not be future-proof, I would like to point out that the ViaVersion API hasn't been changed in the last 3 years (at least getting the player's version), and 3 years ago only the package name was changed.
Another thing that was brought up was "stuff related to other plugins shouldn't be in Skript". Even though I agree with this, I don't think there is another solution right now. So even if this PR doesn't get merged, I propose that we rename or remove this expression because it is straight-up misleading.
Target Minecraft Versions: any
Requirements: none
Related Issues: none