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

added shuffle stack shortcut #132

Merged
merged 7 commits into from
Dec 3, 2022
Merged

Conversation

RomanNovo
Copy link
Contributor

Fixes/Solves
Solved Issue-111
#111

For default key is Z

Copy link
Owner

@drwhut drwhut left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I've just got a few points/questions:

  • The shortcut should be able to be changed in the options menu, under "Key Bindings".
  • In CameraController, we may want to account for the fact the player might be hovering one, or multiple, pieces. If multiple pieces are being hovered, do we allow the player to shuffle all the stacks being hovered, or none at all?
  • Should we only allow stacks to be shuffled when they are selected, rather than if the player is just hovering the mouse over them?

@RomanNovo
Copy link
Contributor Author

  1. Added
  2. In original Tabletop (actually checked right now) Shuffling with multiple pieces selected shuffled them separatly.
  3. Again, in original version, select stack is not required for shuffled. Hover is enough

@RomanNovo RomanNovo requested a review from drwhut November 29, 2022 13:02
Copy link
Owner

@drwhut drwhut left a comment

Choose a reason for hiding this comment

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

Just a few changes needed for the options menu.

game/Scenes/OptionsMenu.tscn Outdated Show resolved Hide resolved
game/Scenes/OptionsMenu.tscn Outdated Show resolved Hide resolved
game/Scenes/OptionsMenu.tscn Outdated Show resolved Hide resolved
game/Scenes/OptionsMenu.tscn Outdated Show resolved Hide resolved
@RomanNovo RomanNovo requested a review from drwhut November 29, 2022 18:24
Copy link
Owner

@drwhut drwhut left a comment

Choose a reason for hiding this comment

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

Think the tooltip needs to be a little bit more descriptive.

game/Scenes/OptionsMenu.tscn Outdated Show resolved Hide resolved
@RomanNovo RomanNovo requested a review from drwhut November 30, 2022 21:12
Copy link
Owner

@drwhut drwhut left a comment

Choose a reason for hiding this comment

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

Thank you! I'll test it on my end before approving (including the echo, I need to check the server is not overloaded with requests).

@drwhut
Copy link
Owner

drwhut commented Dec 2, 2022

With the echo events, the client sends requests to the server around every 30ms, which is too fast in my opinion.

We can either:

  • Keep detecting echo events, but add a timeout to request_shuffle on the server side (e.g. 500ms, or 1000ms?)
  • Not detect echo events.

@RomanNovo
Copy link
Contributor Author

Screw this. Save idea for later. Will delete echo event

@RomanNovo RomanNovo requested a review from drwhut December 2, 2022 20:28
Copy link
Owner

@drwhut drwhut left a comment

Choose a reason for hiding this comment

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

Looks good, will test again before merging.

@drwhut drwhut merged commit 77ddc27 into drwhut:master Dec 3, 2022
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