-
Notifications
You must be signed in to change notification settings - Fork 9
Fix recipe autocompletion #499
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: master
Are you sure you want to change the base?
Conversation
Seggan
left a comment
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.
ngl I'm very skeptical of this code. A lot of it seems to be copied from Paper/NMS, which has many problems, most importantly licensing. Also, this is frankly a huge amount of code touching NMS (700 lines :monkaS:), and IMO it might just not be worth it to have to debug this code every time we update.
nms/src/main/kotlin/io/github/pylonmc/pylon/core/i18n/packet/PlayerPacketHandler.kt
Outdated
Show resolved
Hide resolved
nms/src/main/kotlin/io/github/pylonmc/pylon/core/i18n/packet/PlayerPacketHandler.kt
Outdated
Show resolved
Hide resolved
If we need to be fully fair, we can't even release pylon under LGPL because it is based on paper which is GPL, the main problem might be NMS but again, same logic applies of grayarea Yeah i agree that it is a huge amount of code, but keep in mind most stuff is dead code i need to nuke, as I already wrote, so it probably will end up becoming half that, yeah it might be a pain to mantain still |
|
NMS is not a gray area, there's a reason why Bukkit was taken down and why Spigot and Paper only open source patches. Licensing copied code is absolutely a concern. |
No that was not the case, bukkit went down due to the fact that it was an open source project that was secretly owned by mojang, and once the news hit the public a major contributor DMCAd the whole thing down, also if we take down every project that uses nms then we gonna take down all mods Yeah ik some of the code is copied, and honeslty we can fix that part, I plan on rewriting it, I just wanted to check if it worked or no my base idea |
|
There's a difference between using NMS and copying NMS |
77e71bf to
ff2262b
Compare
|
All issues have been addressed, ready for review, testing on my side didn't create problems |
|
I would still live to argue for not having this, for the same reasons I outlined in Discord |
|
I think I'd be happy to merge this providing it is documented what the hell is going on. Because it is quite important that this works. Would like second opinions though. |
|
I personally still don't like the idea of 500 lines of NMS code being in Core |
Of course not, no one does. But it seems worthwhile for the bug it's solving |
Seggan
left a comment
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 disagree, I would prefer it as a separate addon w/ mixin, but I ain't gonna block it if everyone else says yes
Seggan
left a comment
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.
Woops wrong button
How am feeling rn https://www.youtube.com/shorts/sURzzeCHUUc
Fixes #475
Requires massive testing, has some dead code that I will remove once it is confirmed this abomination works as expected
Client still thinks some recipes are available however and I think that is a client side thing so we can't fix it