Repository navigation
Windfarer Integration - #63
DerToaster98 wants to merge 39 commits into
Conversation
Intybyte
left a comment
Intybyte
left a comment
There was a problem hiding this comment.
A few nitpicks, also after you did testing let me know if everything works as expected, if yes I will merge, but not sure how much i can upkeep this section as i don't know your plugin like movecraft
| } | ||
|
|
||
| // Check for cannon, if there is one, add it to ourselves | ||
| final Cannon atLocation = CannonManager.getInstance().getCannon(movecraftLocation.toBukkit(craft.getWorld()), playerCraft.getPilot().getUniqueId()); |
There was a problem hiding this comment.
I remember this call not always working but it was some time ago, so let me know after testing if everything works
Also as i was making the advanced schematic detection part of this logic was changed in favour of scanning all the cannons once at pilot and then moving the cannons at translation, maybe with added detection at creation if to add to the ship, so I will probably need to change this later regardless
There was a problem hiding this comment.
Interesting, is there a more reliable way? Also, are those methods safe to be accessed via multiple threads? The way i wrote the detection step it uses multithreading to split the workload and accelerate the step.
What that exact part does in short is that it detects the cannons on piloting as well (it adds an additonal step to detection), then validates what it found once the event fires (happens after it) and then uses the cached objects.
Are the Cannon objects always there or are they created and removed on demand?
There was a problem hiding this comment.
Most of those methods delegate to the cannon manager class, which uses ConcurrentHashMap, so they should be threadsafe for the most part but I never double checked properly
Thank you for the review. I will begin with testing once your nitpicks have been adressed. As for maintenance, personally i think Movecraft doesn't have that much of a future, as far as I can tell from writing the integration here, Windfarer behaves mostly similar to Movecraft. I wrote the integration to make use of newer APIs and to make it more performant with Windfarer. |
Intybyte
left a comment
Intybyte
left a comment
There was a problem hiding this comment.
More nitpicks after another review, after those it should be fine codewise
|
does this close #62 ? so i can link it |
it potentially does, i have contact to that person, so, once i got cannons to build, i'll give them a test build as well |
…s provided dependency
Uses the DataTag API present in modern movecraft!
Added a TODO comment regarding a new approach for cannon detection and noted the current limitations of the cannon cache.
… hitbox; Fix typo in translation handler
|
2 participants
Adds integration code for the Windfarer Movecraft derivate.
Needs to be tested, but it should work in theory. Sadly due to the fact that Movecraft and Windfarer share certain classes, a few little ugly workarounds are necessary.
Let me know what you think of it.