Lurkars / gloomhavensecretariat

A Gloomhaven / Frosthaven Companion App
https://gloomhaven-secretariat.de
GNU Affero General Public License v3.0
179 stars 69 forks source link

Sync Character Attack Modifier Draws to All Clients #391

Closed rdoll closed 1 year ago

rdoll commented 1 year ago

Is your feature request related to a problem? Please describe.

Just played FH with GHS last night and it went well, but we missed seeing each others AM draws. Part of the fun is the cheers on crits and boos on misses.

Describe the solution you'd like

Same UX, just sync opening the AM deck and drawn cards to all clients.

Describe alternatives you've considered

As a stopgap improvement, we could just flash the drawn cards to other clients, either in the characters nameplate or in/near the footer.

Additional context

If you aren't already working on or planning to work on this, I may be able to help. Please feel free to describe any solution nuances or details that could help.

Lurkars commented 1 year ago

So the draws should be already synced. The only thing that's not synced (on purpose) is the expanded state. This is mainly for the use case of people managing their deck on mobile and having a main screen that's shouldn't get distracted. To get what this issue is asking for, every client who wants to see the deck can just expand it with one click. So changing this can break other's usage. The only solution could be adding a setting for this, but this will require some server and logic changes. So I would first ask if you have considered using the "Permanent Character AM Deck" setting and just expand the deck on every client once. Maybe this is enough for you.

rdoll commented 1 year ago

Great info, thanks. Permanent AM deck is the way to go today, but there may be some alternatives.

Solutions with shortcomings:

Possible solutions:

I'm leaning towards flashing the drawn AM cards inside the character's nameplate, but lemme know what you think. This has enough complexity that I don't want to rush anything that might negatively impact the UX. I don't know the roadmap, but we could also park this if other things have priority. But if you are up for it and we can arrive at a good approach, I'm game for trying :).

Lurkars commented 1 year ago

So I played around a bit yesterday and just added a setting to enable/disable sync the opened state. If disabled on a client, the deck state is just not synced. In general this is working fine and should solve this issue. Now I only have one problem:

So User A has sync enabled, User B not

Not sure if this is a real problem when using the app, was just my test environment, where I run it and it felt just really weird controlling the others AM deck state without any visual state change (because AMD sync disabled). Maybe you can also put some thoughts in this.

General to your feedback: I really like the idea with the flashing cards, will think about this also.

rdoll commented 1 year ago

Ah, right. If all multi-client events are communicated as game state changes, we'd end up with a lot of game state changes that effectively do nothing but clutter up the undo stack. Alternatively, if we added the ability for clients to broadcast non-game state events to each other (through a new server endpoint), clients that joined after the broadcast will start "out of sync". I'm not a fan of either solution.

So that leaves nameplate flash and footer flash. I slightly prefer a nameplate flash, but I think a footer flash would work well too. The trick is to make the flash alert eye catching, yet unobtrusive, while being readable on a mobile.

Slightly different idea: what if there was a client local setting for "show the current character's AM deck below their nameplate", which can only be enabled if "Permanent AM deck" is enabled? Since there is only one currently active character, this alleviates the vertical space concerns and ensures all remote draws are shown. This doesn't solve every client having to enable it to see remote AM draws, but this could be done in addition to flashing a remotely drawn AM card.

Regardless of "show the current character's AM deck below their nameplate", nameplate and/or footer flashing could be useful. Let me know how you want to proceed. Thanks.

Lurkars commented 1 year ago

I really like the idea with the active characters toggle. Directly tested it now and will come later! Seems at least to be the fasted solution to solve the problem in general for you.

And yeah, as said besides all that, I also like the flashing idea, but this will be put to backlog for now I think.

rdoll commented 1 year ago

Awesome! I'll look forward to those changes and I'll be sure to validate them.

And thanks for being so willing to hear out new ideas. Your efforts are appreciated.

Lurkars commented 1 year ago

You can directly checkout the new v0.77.2. The setting is directly under Permanent AM Deck as Autom. active AM Deck. Again happy about feedback and I'll keep this issue open anyways for the flashing.

rdoll commented 1 year ago

Validated it locally with one client, with two clients and with someone helping remotely. It works perfectly and exactly as designed. I even noticed that if a client manually opened the AM deck, the auto-close when the character is no longer active does not close the AM deck. That feels like a nice touch for clients that, for example, always want their AM deck open.

I think the Auto. active AM Deck option is closed in this issue. If you want to close this issue, I can open a new one just for flashing. Or I'm happy to keep this open too. Either way -- thanks for the feature add. I hope others enjoy it as much as me and my group will!

Lurkars commented 1 year ago

Added a draw animation on closed character AM decks in v0.77.5. Can be disabled in settings. Happy about feedback.

rdoll commented 1 year ago

This is fantastic; it's just what I had in mind! I validated it and it looks great.

My only note is that I found that 2500ms felt a bit better than 3500 millis, esp. when simulating Advantage/Disadvantage drawing. I can do a PR for that, but I suspect you'll want to see how it feels for yourself, so probably just as easy for you to do it while there -- if not, lemme know.

Thanks!

Lurkars commented 1 year ago

I'll check and pay around. My initial thought was, that it should be long enough, that if you're not focusing and see "some movement" on screen, that's it's still long enough to recognise the card. Of course that come to the price, that's it's long if you're already focusing.

Lurkars commented 1 year ago

Okay, I played a bit around and confirm now that 2500ms felt better! So changed now in v0.77.7. Please verify and close.

rdoll commented 1 year ago

Verified. And I also verified the bug fix for the AM draws showing on the nameplate when the permanent AM deck was shown. I was gonna fix that, but you beat me to it! 😄