Skip to content

Use Velocity VarIntFrameDecoder#783

Closed
xism4 wants to merge 5 commits into
PaperMC:masterfrom
xism4:master
Closed

Use Velocity VarIntFrameDecoder#783
xism4 wants to merge 5 commits into
PaperMC:masterfrom
xism4:master

Conversation

@xism4
Copy link
Copy Markdown
Contributor

@xism4 xism4 commented Nov 17, 2022

Velocity provides a pretty frame decoder that could be used without any problem in Waterfall (Also works on java 8)

After debating for some time, I want to continue improving Waterfall to the best of my knowledge. Since I work with it and many people are still using it.
And if anyone wonders why if Velocity already has it? The same question should be asked with other patches that Waterfall uses from Velocity as well.

@Janmm14
Copy link
Copy Markdown
Contributor

Janmm14 commented Nov 18, 2022

Better in terms of what?
Why create unneccessary additional object?
Did you create and run benchmarks?

Additionally there is a licensing problem, as in Waterfall is MIT while the code you want to insert here is GPL.
Or is that code in Velocity written by you originally?

@xism4
Copy link
Copy Markdown
Contributor Author

xism4 commented Nov 18, 2022

Better in terms of what? Why create unneccessary additional object? Did you create and run benchmarks?

Additionally there is a licensing problem, as in Waterfall is MIT while the code you want to insert here is GPL. Or is that code in Velocity written by you originally?

I was talking to a dev from velocity, and he told me that if I followed the license, yes i could open this pull request.

Additionally I have done a couple of tests and it has been a little faster with the new patch.
Above all, a much cleaner source

@Janmm14
Copy link
Copy Markdown
Contributor

Janmm14 commented Nov 18, 2022

Better in terms of what? Why create unneccessary additional object? Did you create and run benchmarks?
Additionally there is a licensing problem, as in Waterfall is MIT while the code you want to insert here is GPL. Or is that code in Velocity written by you originally?

I was talking to a dev from velocity, and he told me that if I followed the license, yes i could open this pull request.

Additionally I have done a couple of tests and it has been a little faster with the new patch. Above all, a much cleaner source

You did not handle the GPL license correctly.

For inclusion of GPL code in a project, the complete project has to be released under GPL license.

This would mean that Waterfall has to change its license from MIT to GPL and may not be published under MIT license anymore.

You should talk with a maintainer of Waterfall before you do so. As I am not sure whether a change to a far more restrictive license for Waterfall is desired here.

@xism4
Copy link
Copy Markdown
Contributor Author

xism4 commented Nov 18, 2022

This would mean that Waterfall has to change its license from MIT to GPL and may not be published under MIT license anymore.

I talked with Five and i understond this, but ill wait @electronicboy, and let's see.

Comment thread BungeeCord-Patches/0066-Use-Velocity-VarintFrameDecoder.patch
@xism4 xism4 requested a review from Kamillaova December 8, 2022 10:55
Comment thread BungeeCord-Patches/0066-Use-Velocity-VarintFrameDecoder.patch
Comment thread BungeeCord-Patches/0066-Use-Velocity-VarintFrameDecoder.patch
Comment thread BungeeCord-Patches/0066-Use-Velocity-VarintFrameDecoder.patch
-{
+public class Varint21FrameDecoder extends ByteToMessageDecoder {

- private static boolean DIRECT_WARNING;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The patch 0044-Don-t-use-a-bytebuf-for-packet-decoding.patch should be edited if you want to remove this line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it

Comment thread BungeeCord-Patches/0066-Use-Velocity-VarintFrameDecoder.patch
@xism4 xism4 closed this Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants