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 ViaVersion Support #6616

Open
wants to merge 8 commits into
base: dev/feature
Choose a base branch
from

Conversation

erenkarakal
Copy link
Member

@erenkarakal erenkarakal commented Apr 26, 2024

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

@sovdeeth sovdeeth added enhancement Feature request, an issue about something that could be improved, or a PR improving something. up for debate When the decision is yet to be debated on the issue in question labels Apr 29, 2024
@sovdeeth
Copy link
Member

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.
As far as viaversion support, this feels like a great thing for an addon to support.

Copy link
Member

@Moderocky Moderocky left a 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.

@erenkarakal erenkarakal requested a review from Moderocky April 30, 2024 16:35
Copy link
Member

@Moderocky Moderocky left a 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

@sovdeeth
Copy link
Member

Request for more reviews/opinions

Comment on lines +47 to 49
if (Skript.classExists("com.destroystokyo.paper.network.NetworkClient") || viaVersionExists) {
register(ExprPlayerProtocolVersion.class, Integer.class, "protocol version", "players");
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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");

@sovdeeth sovdeeth added the 2.10 Targeting a 2.10.X version release label Dec 17, 2024
@sovdeeth sovdeeth removed the 2.10 Targeting a 2.10.X version release label Jan 1, 2025
@Moderocky Moderocky removed the up for debate When the decision is yet to be debated on the issue in question label Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants