schlosrat / token-health

FoundryVTT module
5 stars 10 forks source link

Shortcuts are restricted to GM only #64

Closed DarKDinDoN closed 2 years ago

DarKDinDoN commented 2 years ago

Hello,

FVTT 9.238 5e 1.5.6 Token-Health 0.5.1

As a player, I can not use the module shortcuts. I think this is because all shortcuts are declared "restricted" to the GM: https://github.com/schlosrat/token-health/blob/34d7b6d6bed185c2212a887e644e7c2d4699f60d/token-health.js#L1209

schlosrat commented 2 years ago

This is the intended behavior currently. I think what you're asking for is a new capability to allow the GM to configure TH so that players can launch it. I suspect this would be fairly easy to do, and can imagine there may well be GM's either crazy or trusting enough to allow their players to run around freely doing any amount of damage or healing to each other or NPCs whenever they like. ;-)

I'll look into rolling this into the next version. If it's as easy as I suspect it is, then you can count on seeing it fairly soon.

DarKDinDoN commented 2 years ago

@schlosrat Hello,

It is intended not to allow players to user Token Health on their own tokens? Don't really have in mind players healing or damaging other tokens with TH. I just want them to be able to subtract or add damage easily with TH when I, the GM, told them to do so.

I think previous versions of TH allowed players to heal/damage their own tokens.

schlosrat commented 2 years ago

I've got this all but done. In the next version (0.5.3) the GM will be able to control restricting Token Health or not. The only issue remaining is that if players are allowed to launch TH and a player targets a token they don't control to apply damage or healing, then TH is not smart enough to realize they can't so it will attempt to do as they say. This results in TH generating all sorts of chat messages without actually affecting the token the player is not allowed to affect. This also causes Foundry errors to pop up on the screen when the player tries to do this. The player can, in this case, apply damage or healing to their own token using TH.

I'll need to add some sort of check to TH so it will be smart enough to realize that it's been launched by a player and that the account which has launched it doesn't have permission to modify the targeted or selected token(s). Ideally, I think it would just filter the targeted/selected tokens to eliminate any tokens the launching account lacks permission to modify, and if that results in an empty set then it just bails out entirely without bringing up the dialog box.

I'm not sure how to do this last part yet, and I hesitate to release something that will be known to generate errors as well as confusing chat messages - but we're close.

DarKDinDoN commented 2 years ago

@schlosrat on the last part. Maybe you can default on self? If the player targets an enemy (i.e: a token he can't control), then TH recognizes that and defaults to the player token (selected token).

This way, the user don't have to target to make an attack, untarget to deal damage or heal to self, then retarget an ennemy and so on.

schlosrat commented 2 years ago

I think this is all sorted out now. Generally, the player should probably have their own token selected (or at least only tokens they have ownership of and so can affect), but may have some other token(s) targeted. In that case, they would be able to use the keybindings for applying damage/healing to selected tokens to affect themself without worrying about which tokens they have targeted. If they choose to target their own token they can then use the keybindings for applying damage/healing to targeted tokens.

In any event, Token Health will filter the list of tokens to only those they own, so if they targeted themself and 19 other tokens they don't own then launch Token Health, only their own token would actually make it into the list for those to be affected.

Will this work for what you're intending?

DarKDinDoN commented 2 years ago

Yup, all good I think! Thx

DarKDinDoN commented 2 years ago

@schlosrat Actually, I see the new option. It's correctly unchecked (tripled checked that), even restarted the server. But the player can't open TH with the key "Enter".

schlosrat commented 2 years ago

I noticed something like this on my system too, and I suspect it's a foundry bug. What I found that worked was to go into the configure controls and edit the keybindings from the player account. It didn't seem to matter if I was editing them without actually changing anything or not, though I think the Enter key may be getting intercepted by the browser. Try setting to some other key like "I", etc.

Since my code is correctly making the keybindings available, and the problem goes away when you interact with the FVTT capability to edit keybindings, that makes me think the problem is on their end - but of course, it could be that I'm still not quite doing keybindings correctly.

Please give this a try and let me know if it works for you.

DarKDinDoN commented 2 years ago

@schlosrat ok, tested. It is working as soon as you don't reload the page. Which is unfortunate to ask the players to redo the configuration after each reload.

Noticed that this bug is non-existent on modules that also use keybinding for players & GM, like: https://github.com/manuelVo/foundryvtt-drag-ruler

schlosrat commented 2 years ago

Thanks, I'll take a look at that example and see if I can find out what might be causing the behavior in TH. I agree it's very annoying and shouldn't be like that. At least now it can be launched by players as well as GM.

schlosrat commented 2 years ago

I took a look at the example you listed and see that it's actually hardcoded to always allow the player to launch (restricted: false). Thus, there is never a situation where the player can't launch that module. This is different from Token Health where I allow either behavior - restricted to GM or available to all.

Since the incorrect behavior appears to be on the FVTT side I've posted a bug report for it. From what I can see TH is correctly setting the "restricted" field based on the module setting, and when restricted is set to false that does indeed make the keybindings visible to and configurable by the player. The fact that FVTT doesn't make them functional for the player until after the player has edited them is not a TH bug, it's a FVTT issue.

DarKDinDoN commented 2 years ago

@schlosrat well a simple quick fix would be to always allow the players to use the hotkey and just return false if the setting is active no et the player is not the GM? Cause we might wait for a loooooong time for the FVTT fix

schlosrat commented 2 years ago

That may also be trickier than it sounds.

For that to work, the module needs to determine who has launched it and then act accordingly. That may be possible, but I've never looked into it so I don't know how well it would work. It would also tend to create confusion since the player would be able to see the keybinding settings, and even update them, but not be able to get them to work. I would expect periodic pings from players telling me things aren't working because the module doesn't launch when they press the key they've configured.

Alternatively, I could have it send snarky chat messages of "You're not worthy!" or so other BS, but I think even that would lead to confusion.

DarKDinDoN commented 2 years ago

@schlosrat Ahah love that last sentence, great idea, let's do that :D

DarKDinDoN commented 2 years ago

@schlosrat Followed the issue on FVTT gitlab. Not sure if they intend to change anything in the core related to the issue. Do you think you can implement a fix while the core team changes things?

schlosrat commented 2 years ago

@DarKDinDoN, I've experimented with a way to limit launching the dialog box based on game.user.isGM and the Token Health configuration settings, but this works very poorly. If the Token Health configuration setting is set to allow players to launch it, then they'll be able to launch every time. If it's set to restrict launching, there will be one snarky "You're not worthy!" message, and thereafter no key binding the player presses will trigger a snarky message or bring up the dialog box - they get just the first message. The player can go in and change their keybindings and still not get the message again - so chances are it would pop up once and they'll miss it, then they'll never see it again.

Worse, if the GM then switches the setting back to allow them to launch, they're still locked out without even a snarky screen message to tell them what's going on. That right there is a dealbreaker. Any method that we try here has to be able to switch back and forth at the command of the GM somehow - and this doesn't. Once that first snarky message is displayed the player (possibly all players) are locked out for good.

I've experimented with this check inside the displayOverlay function and earlier in the chain in the toggle function which calls displayOverlay. I get the same result each way. It's very mysterious to me, but I suspect has something to do with FVTT compiling token-health-images.hbs or something else related to how handlebars are used. This is not an area I have any real depth in - that part was done by the original author and is still a mystery to me. Before taking over this project I'd never even heard of handlebars.

At this point, the very kludgy approach of toggling the restricted setting for the keybinding itself appears to work better. Once you get it set the way you want it should stay that way at least, and it doesn't get into the hot mess of once it's restricted it can't be re-enabled.

If you'd like to fork this module and give it a go I'll be happy to roll in a fix you develop that lets the GM switch this on or off reliably, but for now, I'm at a loss for how to fix it.

DarKDinDoN commented 2 years ago

@schlosrat Thx for the detailed report. What about simplifying the module for now?

Two simple rules, nothing fancy:

Shortcut is available for all. If a player tries to launch TH on a token he doesn't own, the module just ignore it (return false;) In any case, we shouldn't have this issue, cause a player that doesn't own an actor/token shouldn't be able to select it in the first place.

schlosrat commented 2 years ago

Fixed in 0.5.4

Module controls now correctly allow GM to permit or restrict players launching Token Health. If the setting is set to restrict players from launching Token Health, they'll get a snarky message telling them they're not worthy on the UI, but the dialog box won't be displayed. Players are also able to set their own keybindings - which they may do regardless of if they're able to actually launch the module or not.

DarKDinDoN commented 2 years ago

Hi, Thx for the work on that feature! Just tested it, it's working great :)