-
Notifications
You must be signed in to change notification settings - Fork 89
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
Support Spigot 1.8 #834
Support Spigot 1.8 #834
Conversation
Wow, thanks @cswhite2000, also hello again! I had a conversation in Discord about this a while ago, but (if it helps) we can target Paper 1.8. I won't have time to review this until the weekend, but will tag @Pablete1234 to take a look as I think he may have some opinions/ideas. |
Also, since you've dived into this: how feasible is it to no longer compile against SportPaper? What would you see as the hurdles? |
@Electroid Also excellent job @cswhite2000 🎉 |
No longer compiling against SportPaper is feasible. You would lose a few features that require sportpaper, and that can't be replicated with reflection. There's also probably a number of places that use SportPaper APIs that I've missed, as in things not used by the default maps, and those would have to handled too. And then it would be fairly easy to go from checking to see if SportPaper is running before running SportPaper specific methods to just removing those method calls. I think ideally, pgm should compile against Paper, but check before using paper specific methods, so that it still runs on spigot. |
So, basically we need a way to listen to events that may or may not exist in the Bukkit runtime. What if in Thoughts? |
That sounds like it would work. |
util/src/main/java/tc/oc/pgm/util/collection/ImmutableMaterialSet.java
Outdated
Show resolved
Hide resolved
This doesn't seem to work. |
What doesn't work about it? Is the package the same? |
You could make a separate PGM module that is pgm-sportpaper, that compiles against sportpaper and adds listeners to sportpaper events and launches PGM events. |
Yeah the package is the same. I copied BlockFallEvent into util but I still got |
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 feel like every place where BukkitUtils.isSportPaper()
is used should contain a comment about what happens about it.
Some features look straight-out disabled, like attribute kits or canplace/candestroy. If these do not work without sportpaper we either need alternatives for other platforms or we need to make it very clear in the documentation that these features will simply not work outside sportpaper.
It's essentially turning them into second-class features that maps could avoid because they don't work, meaning maps may try to avoid using them for being "non-compliant" with the "standard set of features that works everywhere". Should definetly try to look into alternatives.
One decent way to do this would be to move some of these into NMSHacks, and have differing implementations for NMSHacks, one that can work for spigot via NMS or reflection if needed, and another that can make those methods use directy sportpaper apis.
Are you adding the event in that package inside PGM? |
Yes |
I've added more comments explaining these. Attributes and canplace/candestroy should be doable with reflection, however they would take a significant amount of code / time, so I should think they should be done in their own pull request.
That does sound like a good way to do it |
core/src/main/java/tc/oc/pgm/modules/ProjectileTrailMatchModule.java
Outdated
Show resolved
Hide resolved
I believe this is why it didn't work |
Yep, that’ll be the problem. Can we load the class outside of PluginClassLoader? |
I don't think you'll ever be able to load those classes. Bukkit/spigot load plugins with its own class loader, the PluginClassLoader. Due to class loader hierarchy if you try to create your own class loader in PGM to boot those classes yourself, the parent class loader will still prevent them from going thru. |
d5e6280
to
9f5f108
Compare
So at this point pgm should run on sportpaper with all of the features it had before. With spigot I haven't implemented the following:
|
I can't think of any good ways to call any of the events in tc.oc.pgm.util.event.sport when the server isn't running SportPaper. |
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.
This is really great work, @cswhite2000. You're taking this in the right direction, but I think we need more abstractions (separate interfaces/implementations).
Right now, a lot of the pattern is:
if (BukkitUtils.isSportPaper()) {
// SportPaper stuff
} else {
// Fallback, sometimes invoking NMS
}
I worry about too much of that in the codebase, because we have two medium-term goals:
- Don't depend on SportPaper API (reflection only)
- Potentially support newer server versions
Obviously, we aren't going to get there 100% in this PR, but we need to keep that in mind.
For most of my comments, I suggest moving the logic into a utility method (either in BukkitUtils
or NMSHacks
depending on what it does). But for Attribute
, I think we should try a interface/implementation approach (you can take some inspiration from ItemTag
)
It would be great if we no longer import the SportPaper
APIs, and just use reflection, but if you think that's too much for now, we can punt that to a v2.
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
81c8d59
to
68a3829
Compare
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
This was previously called on PlayerResetEvent, but the method that calls that event already resets player attributes Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
core/src/main/java/tc/oc/pgm/inventory/ViewInventoryMatchModule.java
Outdated
Show resolved
Hide resolved
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
This now depends on Electroid/SportPaper#66 Also, this should make multi version support easier since later versions don't support BaseComponents for the tab list either |
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
Some people will try using an older version of |
…Paper Signed-off-by: cswhite2000 <18whitechristop@gmail.com>
It'll fall back to the spigot compatible version if the field doesn't exist |
This still compiles against SportPaper, but I've changed enough of the references to SportPaper specific methods for it to be able to run the maps in https://github.com/PGMDev/Maps without issue. I haven't as of yet tested this on maps not in that repository.
I wasn't really sure how best to handle EventHandlers for events not in Spigot, so I've commented those out for now.
I'm also not sure about how best to handle attributes in spigot 1.8, since those weren't added to spigot until 1.9, so those are also commented out for now.