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

Bukkit 1.10.x - 1.12.x and 1.13.2 and performance / readability improvements #12

Open
wants to merge 2 commits into
base: bukkit-api
Choose a base branch
from

Conversation

2008Choco
Copy link

This PR makes a variety of changes to the plugin in order to have it function for 1.13.2 and onward without requiring any further updates.

  • Adds support for Minecraft 1.13.2+ (ONLY THE LATEST VERSION)
  • Update the Java version to Java 8 (it has become standard in the Minecraft community). Near 100% of servers use it, or otherwise they've updated to Java 9+
  • Various changes to the pom.xml(s)
    • Removed the repository directing to an illegal mirror of the CraftBukkit and Spigot server jars
      • To obtain the server jars, BuildTools should be run and it will be locally installed to the .m2 repository
    • Removed redundant <type>jar</type> declarations
    • Removed the ../target build directory as it is deprecated by Maven and unsupported by Eclipse
    • Removed unnecessary dependencies on the Spigot API and Bukkit API. Spigot-exclusive APIs were not used and Bukkit is shaded into CraftBukkit
    • Removed redundant declarations of groupId in children poms
    • Update the Maven Shade Plugin from 2.4.3 to 3.2.1
    • Update generic dependencies to 1.13.2 (as version-exclusive features are not used, but having the latest version retains project maintainability)
  • Added WildAnimalHandlerSafe as a default implementation used by 1.13.2+ servers... this uses the new API methods

The following are changes made exclusively to specific classes

  • WildSex.java:
    • Declare lastMateAnimals as a Set rather than a Set for memory leak reasons and clear it on disable
    • Let wildSexTask be a BukkitTask rather than an int. These scheduler methods are deprecated, therefore BukkitRunnable instances should be used instead
    • Removed some unnecessary fields that were not used outside of the onEnable() method
    • Ensure no NullPointerExceptions are thrown on disable for unsupported servers
  • WildSexTask.java:
    • Added a ELIGIBLE_MATES_BUFFER List constant to buffer entities rather than creating new ArrayList instances for every iteration.
      • Slightly increases performance and reduces the overhead of ArrayList resizing by defining an initial size of 64
    • Unnested numerous conditions... better readability
    • Use streams in place of for loops where it makes sense (i.e. entity filtering and buffering)
    • Extract the eligible mate condition to a method
    • Use Math#max() instead of ternary in #getReproductionProbability()
  • WildSexTaskListener.java:
    • Remove the unencapsulated retrieval of lastMateAnimals and use proper methods instead
    • Declare variables for father and mother
      WildAnimal.java:
    • Declared a LOVEMODE_TICK_DURATION constant for readability sake in version-specific implementations

This PR also removes the dependency-reduced-pom.xml which was left in the repository by mistake. This is an IDE-specific file and should not be present.

@2008Choco
Copy link
Author

The reason the Travis build is not working properly is due to the local dependency on CraftBukkit and Spigot, though this should be excused for the sake of complying with Minecraft's EULA of not distributing the server jar (which is being done by the mirror repository). I would advise finding an alternative such as a private repository for Travis, but the server jar should not, under any circumstance, be distributed by a Maven repository.

@cgokmen
Copy link
Owner

cgokmen commented Mar 6, 2019

This is a very comprehensive PR and I really appreciate the time you put into it - I am not around as much as I used to be so I really appreciate the contribution.

The changes mostly look good to me and you're definitely right about the pom.xml dependencies. I am also happy that Spigot finally appears to have set up a proper API for the love mode, saving us the trouble of adding updates without new features just for new version matches. Have you had a chance to test this with any of the earlier versions? It definitely will compile but I'm not knowledgeable as to what happens when the plugin depends on an API level at compile time that is not available at runtime. If there are any issues regarding this it can be fixed by putting this futureproof version as a subproject like all of the NMS ones.

Before we merge this PR though, I would like to take a couple of days to try and find time to integrate BuildTools into the travis flow so that it can build the plugin without needing to find the server jar on a maven repository. This should be fairly straightforward but it's important for this to function right since automatic releases on GitHub are set up using travis.

Also, I wanted to point out that if you are interested in becoming the plugin's official maintainer (or at least a contributor), I would be happy to add you as a contributor onto the repo and the plugin page.

@2008Choco
Copy link
Author

2008Choco commented Mar 6, 2019

I am also happy that Spigot finally appears to have set up a proper API for the love mode, saving us the trouble of adding updates without new features just for new version matches

It's something I pull requested to Bukkit for this very reason. Someone on the SpigotMC forums was looking to update this plugin themselves to 1.13 so I figured I'd take the liberty to do it for them and have it be future proof so others wouldn't have similar issues

It definitely will compile but I'm not knowledgeable as to what happens when the plugin depends on an API level at compile time that is not available at runtime

If for some reason the plugin is run on a version of Bukkit that do not have this API, (say 1.13.0, 1.13.1 or early versions of 1.13.2) because these people will inevitably exist, the plugin will throw a NoSuchMethodException. While not ideal, Bukkit doesn't really provide a friendly way of checking API version by commits. Yes, there is Bukkit#getBukkitVersion(), but this returns a commit hash which isn't exactly easy to compare whether one precedes the other. I'm open to changing this if a solution is provided, but as far as I'm aware it's not possible.

That being said, server owners are expected to update their servers regularly (hence the reason there is a server startup delay for server builds older than a few days).

Also, I wanted to point out that if you are interested in becoming the plugin's official maintainer (or at least a contributor), I would be happy to add you as a contributor onto the repo and the plugin page.

In truth, I'm not sure the project will require a maintainer if this PR were to be merged. Due to this project's simplicity and the fact that there is now a proper API for this, it's not likely it will break in the future. Bukkit strives to be as forwards compatible as possible.

@DrkMatr1984
Copy link
Collaborator

Good job, Choco. I am not around as often as I used to be to help Sultan also. Many people request this plugin from me still on Bukkit and Spigot and I'm sure they will be ingratiated to see updates.

@cgokmen
Copy link
Owner

cgokmen commented Apr 11, 2019

Sorry, I realized I dropped the ball on this again, I have some time this weekend that I intend to use towards this end.

I believe I know how to leverage the current setup to use the Bukkit API for newer versions and our makeshift API for older ones. Will try that, and once that is done we should hopefully have full backward and forward compatibility.

@cgokmen cgokmen changed the base branch from master to bukkit-api May 19, 2020 04:12
@cgokmen cgokmen mentioned this pull request Mar 17, 2021
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.

3 participants