search

sPOiDar / fvtt-module-stream-view

FoundryVTT module that provides a minimal UI view with automated camera work, ideal for streaming or recording games.
MIT License
11 stars 11 forks source link

Various improvements #41

Closed cs96and closed 1 year ago

cs96and commented 1 year ago
  1. Update the automatic stream view when a new token is added to a scene
  2. I set my stream user to be Owner of all player tokens, which lets me select those tokens on the stream view to show their line of sight. However, this meant that the isVisible calculation in _combatTokens() was not returning true, if one token was selected on one side of a wall, and the next combatant was a player on the other side of that wall. I have fixed this by forcing Stream View to release all selected tokens at the start of a turn.
  3. Added a new option to automatically select the current combatant's token at the start of their turn.
  4. Added a control to temporarily enable/disable stream view updates.
sPOiDar commented 1 year ago

Sorry for the delay in reviewing, haven't had a lot of time for personal projects this year.

The number of changes in this PR makes it somewhat difficult to review cleanly, is there any chance you can split these features into individual PRs?

Generally, I have the following feedback:

  1. This is likely an easy merge
  2. I'll have to do some testing on this, I don't particularly like the way this has been fixed here
  3. This is probably mergable, though as above I'd like to do some testing with OWNER perms, as this is not the standard way the module was intended for use.
  4. This feature addition seems fine, however for implementation I would probably have opted for an additional CameraMode rather than a new property, which would likely clean up some of the new logic you had to add to support the new flag.

If you're able to submit new PRs, please update the commit messages to follow the Conventional Commits specification so that they're picked up by my CI correctly (see the commit history on the repo for examples).

Thanks for your efforts!

sPOiDar commented 1 year ago

1. I've cherry-picked and committed this change as 19126008f7f055d1d0f0c0e9586c4724892e1d1b 4. This was implemented in e725bddbfc4d283d822c27663b85e8247f9254dc

cs96and commented 1 year ago

Do you mean 1 and 4? Not 1 and 2?

sPOiDar commented 1 year ago

Github helpfully auto-renumbered the list because it detected a numbered list, even though I wrote 1. and 4. :-/

cs96and commented 1 year ago

That makes more sense, thanks!

I've been meaning to split this up into individual PRs but unfortunately I've not had the time to do it recently. I'll see if I can rebase, and submit two new ones for 2 and 3.

sPOiDar commented 1 year ago

No worries, I know how that goes - I've also been very time-poor.

cs96and commented 1 year ago

Will create new PR for #2 and #3 combined (as they are part of the same functionality).