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

Changes to the Java Random Implementation #57

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PhilippCl
Copy link

@PhilippCl PhilippCl commented Aug 8, 2022

This uses the java random implementation instead of the kotlin one. The Plugin is now useable with Minecraft Spigot/Paper 1.19.2

The Error before: https://gist.github.com/PhilippCl/4209b1cfd6ba600ad86fd47ad565a059

@PhilippCl PhilippCl changed the title change to java random & version bump Changes to the Java Random Implementation Aug 8, 2022
@AtriusX AtriusX added the bug Something isn't working label Aug 8, 2022
@AtriusX
Copy link
Owner

AtriusX commented Aug 8, 2022

Personally I'd check and see if you can figure out a way to get the Kotlin implementation working again rather than relying on the JVM implementation as that can get a bit confusing with imports. The way you're using Random in this case isn't ideal either as you would be instancing a new Random object every time you call teleport, whereas before it was just calling a static instance.

Lastly I'd recommend breaking up this into a couple smaller PRs, one for the fix and another to update the API version of the plugin. Would help to keep PRs from getting a bit too messy and hard to track. Should also avoid bumping the version number in this PR too. Thank you!

@PhilippCl
Copy link
Author

This was more like a quick fix for yesterday. Sorry its really messy. I will try to find a solution with the Kotlin Random Implementation and remove the version bumps from this pr after work today. Thanks for the answer and for the nice plugin 👍

@AtriusX AtriusX marked this pull request as draft August 9, 2022 13:43
@AtriusX
Copy link
Owner

AtriusX commented Aug 9, 2022

No problem and thank you for your interest! I've marked this as a draft for now so it doesn't accidentally get merged in until you're ready. Just convert it back to a regular PR once its done. 👍🏼

@AtriusX
Copy link
Owner

AtriusX commented Aug 14, 2022

Just checking back in, any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants