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

Rewrite/rotation task #683

Merged
merged 11 commits into from
Aug 17, 2024
Merged

Conversation

Intybyte
Copy link
Contributor

@Intybyte Intybyte commented Aug 7, 2024

Describe in detail what your pull request accomplishes

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

Checklist

  • Tested

@DerToaster98
Copy link
Contributor

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
Copy link
Contributor Author

Intybyte commented Aug 7, 2024

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
Copy link
Contributor

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

Copy link
Contributor

@TylerS1066 TylerS1066 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I appreciate the work and improvement to the codebase this PR (and it's twin) brings, I would like to avoid PRs that refactor classes without any performance or feature improvements (especially for classes that will need to be rewritten in the near future).

@TylerS1066 TylerS1066 merged commit dc05661 into APDevTeam:main Aug 17, 2024
1 check passed
@Intybyte Intybyte deleted the rewrite/rotationTask branch August 25, 2024 19:18
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