feat(folia): added folia support #6
Conversation
There was a problem hiding this comment.
Thanks for the adjustments but I have some major concerns regarding the compatibility of this implementation especially with older Spigot servers.
I kinda wonder about the current approach of the folia scheduler/task wrapper utils breaking on class load when the Folia-specific stuff does not exist. Did you test that on normal Spigot or a pre-folia Paper?
An approach to use different class adapters based on the server type similarly to how PaperLib works might be better (and even cleaner and less confusing code-wise) than deciding on the fly in each method which code to use depending on the platform. (That would also allow to theoretically modulize the platform adapters so the base plugin can be compiled just against the original spigot-api dependency) I kinda think such a complex approach should be done in a library though but if no proper one exists then that's a bit unfortunate :/
Also while I don't have an issue with using GPLv3+ I'm unsure right now how exactly MPLv2 (which the plugin is currently distributed under) and GPLv3+ work together but I assume it just means it would need to be distributed under GPLv3+. That should get reflected in the repo somehow (in a readme which this still doesn't have 😅) and the license file added if the GPLv3 code is actually continued to be used.
| EntityType.PALE_OAK_BOAT, | ||
| EntityType.PALE_OAK_CHEST_BOAT, | ||
| EntityType.SPRUCE_BOAT, | ||
| EntityType.SPRUCE_CHEST_BOAT | ||
| }, |
There was a problem hiding this comment.
This will break on older server versions. The plugin is at least at the current time meant to be compatible with basically all versions, at least with 1.13 and up (as stated in the plugin.yml) and I also don't think the changing of the search groups fits in this PR.
| import org.bukkit.Chunk; | ||
| import org.bukkit.Location; | ||
| import de.themoep.entitydetection.util.folia.FoliaScheduler; | ||
| import org.bukkit.*; |
There was a problem hiding this comment.
Please do not use wildcard imports, they can lead to errors when a different class is used than expected.
|
|
||
| private Map<SearchType, SearchResult<?>> results = new HashMap<>(); | ||
| private Map<String, SearchResult<?>> customResults = new HashMap<>(); | ||
| private Map<String, SearchResult<?>> lastResultViewed = new HashMap<>(); | ||
|
|
||
| private boolean serverIsSpigot = true; | ||
|
|
||
| public static EntityDetection getInstance() { | ||
| if (instance == null) throw new IllegalStateException("EntityDetection has not been initialized yet!"); | ||
| return instance; |
There was a problem hiding this comment.
I'm not really a fan of introducing a singleton pattern where none existed before. (Especially as it's technically possible to create multiple instances of the same plugin so they aren't really singletons to begin with)
What benefit over simply using dependency injection does this offer?
| <version>1.13.2-R0.1-SNAPSHOT</version> | ||
| <groupId>io.papermc.paper</groupId> | ||
| <artifactId>paper-api</artifactId> | ||
| <version>1.21.4-R0.1-SNAPSHOT</version> |
There was a problem hiding this comment.
Does this break spigot 1.13.2 (and up) compatibility? If so then this might not be the best approach :/ I'd prefer to keep this available as a utility plugin for a wide range of people.
| private EntitySearch currentSearch; | ||
| private TaskWrapper currentSearchTask; |
There was a problem hiding this comment.
I kinda feel like storing the task as a field of the search instead of separate in the plugin could be cleaner. Having two fields representing the current running search feels weird (espcecially as one now needs to ensure they both stay in sync) and how that search is more a detail of the search itself and not the plugin.
There was a problem hiding this comment.
Thanks for all the feedback to this quick fork, however, I'm sharing this code purely for your own plugin's supported platforms.
I need Folia support, because we will start using it on one of my client's server.
In this instance, it works on our end, which was my goal.
That does mean I haven't (and won't) tested it on older version, nor pre-folia builds etc.
Feel free to close the PR if you don't want to further add Folia support to this resource!
Cheers
There was a problem hiding this comment.
Well I'm definitely open to adding support for it however this is a vital tool used by many so any addition needs to make sure that it keeps working for them. Publishing software for others to use unfortunately makes it hard to do radical changes like Folia support is when not implemented properly hence why tho current approach just wont work.
No description provided.