search

APDevTeam / Movecraft

The original movement plugin for Paper. Reloaded. Again.
GNU General Public License v3.0
121 stars 76 forks source link

Rewrite/rotation task #683

Closed Intybyte closed 1 month ago

Intybyte commented 1 month ago

Describe in detail what your pull request accomplishes

Refactoring some methods and moved/added some methods to CruiseDirection.java.

Checklist

DerToaster98 commented 1 month ago

Neat necessary restructuring of this task.

I think it would also be beneficial to move the subcraft specific code into it's own method too, makes it cleaner.

Also you might want to look at your condition for the continue in rotateEntitiesOnCraft() again, probably you are just missing a negation to the contains check. Like it is now you won't move any tnt entity if ONLY_MOVE_PLAYERS is set, iirc previously movecraft always moved tnt entities and player entities regardless of that setting. Might also be worth it changing that hardcoded list to a entity tag or a whitelist in movecraft's main config.

Intybyte commented 1 month ago

Also you might want to look at your condition for the continue in rotateEntitiesOnCraft() again, probably you are just missing a negation to the contains check. Like it is now you won't move any tnt entity if ONLY_MOVE_PLAYERS is set, iirc previously movecraft always moved tnt entities and player entities regardless of that setting. Might also be worth it changing that hardcoded list to a entity tag or a whitelist in movecraft's main config.

Double checking that part of the code, btw this is a rewrite to make it more understandable for now, if needed we can make another PR once the rewrite is done.

Also to be fair that wasn't even a list to begin with, I just made it look like one xD

DerToaster98 commented 1 month ago

Also you might want to look at your condition for the continue in rotateEntitiesOnCraft() again, probably you are just missing a negation to the contains check. Like it is now you won't move any tnt entity if ONLY_MOVE_PLAYERS is set, iirc previously movecraft always moved tnt entities and player entities regardless of that setting. Might also be worth it changing that hardcoded list to a entity tag or a whitelist in movecraft's main config.

Double checking that part of the code, btw this is a rewrite to make it more understandable for now, if needed we can make another PR once the rewrite is done.

Also to be fair that wasn't even a list to begin with, I just made it look like one xD

Now that you mention a separate PR: I have plans to abstract the rotation task more to also add things like a Roll task (mainly for Subcraft Roll to make things like a garage door). For the list, you might want to make that as a constant here to avoid unnecessary object creation.

Same thing for the cruise task btw, cruise direction could be changed to a vector that defines the direction and the cruise step length. But that also belongs in a separate PR