FlansMods / FlansMod

Flan's Mod for Minecraft
Other
236 stars 149 forks source link

Fixing issue #1245 as well as reverting a recent change to the roll mechanic #1253

Closed matt-bel closed 4 years ago

matt-bel commented 4 years ago

Feel free to ignore the first part of the pull request, but I believe I have found a passable work-around for the tank firing bug.

Liruxo commented 4 years ago

First up: Thank you for fixing these issues and contributing to FlansMod. However, i have a few problems with this PR:

The other commit is in my opinion problematic for a few reasons:

To sum up: Please do not use opinionated and undescriptive text in source code comments and commit messages. You are free (and encouraged) to explain your reasoning in the PR comments, as this is the right place to do so

Continuing, i have additional problems with the current solution you proposed: Ideally i would like a config based solution, containing a few more options allowing players to choose the right setting for them. For example i personally would also like to experience roll as passenger. However, i am open for other solutions so please open another PR for this issue so we can discuss the issue there.

ChrisLane commented 4 years ago

I think perhaps you haven't fully understood what your changes do. There are still instances where the same behaviour will occur. The existing code was already a bit of a mess to follow and so I've done some refactoring and fixed #1245 as well. Perhaps you could test the changes and see if you're happy with them.

Sorry it's taken me so long to respond to this. As @Liruxo mentioned, PRs should ideally focus on one thing to make it easier to review and discuss and so I will be closing this.