Roose Bolton doesn’t return discarded house cards on his 2nd use

Longwelwind / swords-and-ravens

An online platform to play the board game "A Game of Thrones: The Board Game 2nd Edition"

https://swordsandravens.net/

GNU General Public License v3.0

108 stars 29 forks source link

Roose Bolton doesn’t return discarded house cards on his 2nd use #451

Closed gereon77 closed 4 years ago

gereon77 commented 4 years ago

https://swordsandravens.net/play/2e581a2d-be2b-467b-a691-4bc6ccce2b35

Turn 9: Battle Lannister vs Stark

norielbit commented 4 years ago

Hey, I looked at your project and was really impressed by the quality and scale! I'd like to help you guys where I can, so I figured taking out stuff in red is a good place to start : P I looked into this a bit and it appears to be Tywin's ability interacting badly with Roose. I believe that I found the reason, as well as some other cases in which the bug occurs such as when Renly is used when Baratheon has max knights or no footmen in the battle (the latter is easily tested to be true by immediately fighting on the Narrow Sea). I figured out a fix over at a fork I opened, and it seems to be working fine as far as I could tell.

What branch should I open a PR to so you guys can properly review it?

gereon77 commented 4 years ago

Hi! Welcome on board! :)

Sounds great. Long and me had no success fixing it with our first tries...

I‘d say create a branch like „fix-roose-bolton“ (kebab case is the convention) and PR it.

BTW: Further dev coordination is done on Discord. Feel free to join the GoT server and dev channel there!

gereon77 commented 4 years ago

I saw your branch but I don’t dare to review it. @Longwelwind will do it anyways before it get merged.

Just two small additional hints. Rebasing on latest master commit is preferred over merging master into the hotfix/feature branch (rebasing keeps the tree clean). And commit messages should be in present tense.

Aside that I am impressed and glad another dev join this project. The list of todos is long and therefore fixes and new features will come faster when more contribute...

Longwelwind commented 4 years ago

Hello @norielbit!

As @gereon77 said:

Write the commit message in the present tense (as defined here
It's better to rebase your branch off master when you want to take new changes into your branches. It's okay for this one, though.

Regarding the content of the branch, it seems to be the correct fix! Hadn't thought of checking those effect in particular!

norielbit commented 4 years ago

@gereon77 @Longwelwind Sorry for the git newbing, I was given a very "hands-on" practical introduction and not enough good habits for it. I read the article you linked, and I'm happy to learn better practice though, so whenever I do a slip-up I'd like it if you guys would tell me (like these points).