-
-
Notifications
You must be signed in to change notification settings - Fork 107
feat. add worldguard support on quests #812
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
|
Hello, i fixed 2 others bugs too, i used it actually on my servers |
|
It's a highly inefficient solution (at least not sufficiently efficient to be accepted in current form). I will take a look into introducing such a feature though. |
|
Hello I don’t understand ? I used it since I publish this PR and everything working well |
It's inefficient. Your code, completely unnecessarily, performs region query for each quest and task started: Quests/bukkit/src/main/java/com/leonardobishop/quests/bukkit/util/TaskUtils.java Lines 321 to 329 in ea2e1e8
Take a look at how biome key check is implemented. It computes the biome only once per the entire method call. With current implementation, for example, 1000 3-task quests with region requirement in each, the method would need to perform 3000 region queries instead of just one (and these are probably considered heavy). Another issue I can see with that implementation is that it would be preferred to use appropriate location depending on task type (for example for mining it should check mined block location and for mobkilling the mob death location), however the issue is already a thing when it comes to the biome check, so not really a big deal. Also, probably the WorldGuard integration should be separated into a dedicated hook (like Essentials/CoreProtect are), so there is no need for these ugly checks and linkage error handling in TaskUtils class. Oh, I also don't really understand the need for these checks, as BlockBreakEvent definitely shouldn't be called for air. Shouldn't it be fixed on other plugin side? Quests/bukkit/src/main/java/com/leonardobishop/quests/bukkit/tasktype/type/MiningTaskType.java Lines 53 to 58 in ea2e1e8
|
Hello,
I implemented worldguard support, it works, i don't test it without worldguard and idk if i do that correctly with your : "compileOnlyPlugin" etc, let me know if i've to change something else, thanks you