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 4 commits into
base: dev/feature
Choose a base branch
from

Conversation

erenkarakal
Copy link
Contributor

@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.

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

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. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants