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

#36 Bugfix - Moving active character up in list no longer lets previous ... #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

samcorcoran
Copy link
Contributor

...character take a second turn

Moving active character from bottom to top of list allows them to continue to have current turn as cyclical order has not changed

…s previous character take a second turn

Moving active character from bottom to top of list allows them to continue to have current turn as cyclical order has not changed
@samcorcoran
Copy link
Contributor Author

Fixes two edge cases in moving the active character. Wherever the character is moved to, the next character in the list is active next. This bugfix accounts for whether that next character's index stays the same (moved character was relocated to location above their position) or decreases (moved player moved down list).

The turn order is cyclical, so if a character is moved from last in list to first in list, their order hasn't actually changed. This could be explicitly disallowed, but at the very least the above bugfix also ensures that, if moved from bottom to top of list, the 'current turn' gets incremented by one to wrap round and end up letting the moved player still be active. I think this is the right way for this to work in regards to in-game rules (as it is a mechanical ordering of the PT list, but is meaningless in the context of the game).

@tsiemens
Copy link
Owner

tsiemens commented Sep 5, 2014

Couple issues:

  1. Your if/else should have curly braces, regardless of the single lined nature of the contents. I prefer to follow this as a rule.
  2. The comments there are not particularly useful, since they just repeat your if statement, so please remove those. However, your description above concerning going from last position to first position not technically being and order change, and thus not warranting an actual turn incrementation would be a good comment, so please add that in the else block

@tsiemens
Copy link
Owner

tsiemens commented Sep 6, 2014

I also forgot, you need to merge this with the release-2.2 branch, not master.

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.

2 participants