Add property to inherit onlyVisibleForSeat while dragging. #1357

flackr · 2022-10-14T03:06:26Z

While dragging, inherit the onlyVisibleForSeat property from the current hover target to hide visibility for changes made over a hidden widget. Fixes #1356.

While dragging, inherit the onlyVisibleForSeat property from the current hover target to hide visibility for changes made over a hidden widget.

ArnoldSmith86 · 2022-10-14T03:06:29Z

PR-SERVER-BOT: You can play around with it here: https://test.virtualtabletop.io/PR-1357/pr-test (or any other room on that server)

After merging, a backup will be available at https://beta.virtualtabletop.io/editor/PR1357-pr-test.

flackr · 2022-10-14T03:07:57Z

Note, this should inherit from ancestor holders too I think. I will add a commit to address this, just wanted to share out the idea. You can try it at https://test.virtualtabletop.io/PR-1357/pr-test

flackr · 2022-10-14T03:23:26Z

Now this should inherit from ancestor widgets as well. I've updated the pr-test to show an example of a nested holder without the property set.

96LawDawg · 2022-10-14T03:24:24Z

This works as described. The latest commit also makes it work for ancestors. I have a demo in https://test.virtualtabletop.io/PR-1357/lawdawg. The left holder has an ancestor parent widget that is different from the holder and that works. The right holder is a demo of it working without an ancestor. Based on testing, I would approve this. Needs a code review.

96LawDawg · 2022-10-14T03:32:39Z

Proposed wiki edit.

Add the following new entries to Global properties for every widget:

Property	Default value	Description	Tutorial
childrenInheritVisibleForSeat	false	Widgets dragged over the hover area of a valid `dropTarget` implement the most restrictive `onlyVisibleForSeat` property of any ancestor with this property set to true. This creates an effect similar to the `childrenPerOwner` property of holders. The property `hoverTarget` is also added to the widget while being dragged.
hoverTarget	null	When the 'childrenInheritVisibleForSeat' property is true, `hoverTarget` is set to the `id` of the holder the widget is over.

flackr · 2022-10-14T03:43:52Z

@96LawDawg FYI note that this is the onlyVisibleForSeat property. Also the ancestor lookup stops once it finds an ancestor with childrenInheritVisibleForSeat == true so you it will never be inherited from an ancestor of the widget with that property set, but it will inherit down to grandchildren. I think you can remove the "and its ancestor widgets".

96LawDawg · 2022-10-14T04:09:44Z

@96LawDawg FYI note that this is the onlyVisibleForSeat property. Also the ancestor lookup stops once it finds an ancestor with childrenInheritVisibleForSeat == true so you it will never be inherited from an ancestor of the widget with that property set, but it will inherit down to grandchildren. I think you can remove the "and its ancestor widgets".

I see what you are saying. But I think we should document somehow that it looks up the ancestor tree. The effect will take place even if THIS widget doesn't have the property but an ancestor does. Open to suggestions on how to say that.

flackr · 2022-10-14T12:57:30Z

How about something like the following?

If set to true, child widgets will inherit the onlyVisibleForSeat property of this widget while being dragged over the area of the widget or any of its descendants. This works similar to the childrenPerOwner property of holders.

Or alternately if we want to more directly match the code:

When dragged, child widgets will inherit the onlyVisibleForSeat property of the nearest ancestor with this property set to true. This works similar to the childrenPerOwner property of holders.

Let me know if anyone has name suggestions for the property, childrenInheritVisibleForSeat is a bit long (and doesn't even include the full name of the inherited property (i.e. onlyVisibleForSeat).

96LawDawg · 2022-10-14T14:36:21Z

Or alternately if we want to more directly match the code:

When dragged, child widgets will inherit the onlyVisibleForSeat property of the nearest ancestor with this property set to true. This works similar to the childrenPerOwner property of holders.

I like a slightly modified version of that: "When dragged, child widgets inherit the onlyVisibleForSeat property of the nearest ancestor with this property set to true. This works similar to the childrenPerOwner property of holders.

Let me know if anyone has name suggestions for the property, childrenInheritVisibleForSeat is a bit long (and doesn't even include the full name of the inherited property (i.e. onlyVisibleForSeat).

How about just inheritVisibleForSeat?

flackr · 2022-10-14T15:20:06Z

Or alternately if we want to more directly match the code:

When dragged, child widgets will inherit the onlyVisibleForSeat property of the nearest ancestor with this property set to true. This works similar to the childrenPerOwner property of holders.

I like a slightly modified version of that: "When dragged, child widgets inherit the onlyVisibleForSeat property of the nearest ancestor with this property set to true. This works similar to the childrenPerOwner property of holders.

Sounds good to me!

Let me know if anyone has name suggestions for the property, childrenInheritVisibleForSeat is a bit long (and doesn't even include the full name of the inherited property (i.e. onlyVisibleForSeat).

How about just inheritVisibleForSeat?

My only concern with removing children is that it suggests to me the holder itself inherits the visible for seat, rather than the descendants dragged over it. I was also considering childrenInheritVisibility, childrenInheritVisibleSeat or variations like that.

ArnoldSmith86 · 2022-10-14T15:20:51Z

How much work would it be to not use this.set? In general, I try to avoid transferring "useless" properties that are basically already known by all clients if you know what I mean.

It might make the code a lot more messy so this might be the preferable solution.

flackr · 2022-10-14T15:22:56Z

How much work would it be to not use this.set? In general, I try to avoid transferring "useless" properties that are basically already known by all clients if you know what I mean.

I think that the other clients don't know the current hoverTarget, right? Computing it (on all clients) throughout the drag would I think add a bit of complexity*.

* unless this already happens?

ArnoldSmith86 · 2022-10-14T15:32:08Z

Oh yeah, didn't think about that.

robartsd · 2022-10-14T15:45:59Z

What about just setting a _hoverTarget property to the id of the current target at the end of the move routine and handling the logic for hiding the widget based on the dragging (set to playname in moveStart) and _hoverTarget properties in applyDeltaToDOM? This would eliminate overwriting the dragged widget's onlyVisibleForSeat property and provide game designers a property to use with a changeRoutine to create custom behaviors (without resorting to complex routines based on x and y).

flackr · 2022-10-14T15:49:15Z

@robartsd I really like this idea. I was considering something like this but wasn't sure whether there was any aversion to adding internal state properties, but I think like you said this property would also be helpful for custom behaviors.

robartsd · 2022-10-14T15:52:59Z

@robartsd I really like this idea. I was considering something like this but wasn't sure whether there was any aversion to adding internal state properties, but I think like you said this property would also be helpful for custom behaviors.

We already have it for dragging (which should probably have been _dragging, but changing it now could potentially break existing games).

Actually, I'm not sure if the _ naming convention should apply. The current _ properties are read-only virtual properties (computed from state, but not actually saved in it); these properties are state properties that also should not be set by game designers, but they are not virtual properties.

flackr · 2022-10-14T20:41:07Z

@robartsd I implemented the suggested approach. It works great - the onlyVisibleForSeat property no longer gets clobbered and we respect the intersection of the onlyVisibleForSeat that was set on the dragged token and the restricted visibility inherited from the container. I left the new hoverTarget property unprefixed for now per your comments but happy to change it.

client/js/widgets/widget.js

96LawDawg

Changes tested and approved. New proposed wiki entry edits suggested above. Please review.

This technique could be used to change the visibility of the cursor with a pretty simple changeRoutine to check for hoverTarget and dragging that wouldn't require a code change. But so far, I can only figure out how to change the cursor transparency for the entire room. @robartsd or anybody ... do you know of a way to change the transparency of the cursor for a specific playerName? BTW, here is how I change the cursor transparency for the entire room.

"css": {
    ",.cursor": {
      "--cursorActiveOpacity": 0,
      "--cursorPressedOpacity": 0
    }
  }

flackr · 2022-10-15T06:01:11Z

FWIW I still feel like it would be a sensible default behavior for the drag visibility for others to match what it would be after dropping a dragged item, however adding this property to holders as well as onlyVisibleForSeat for games where you want to explicitly choose this behavior works.

96LawDawg · 2022-10-25T18:27:48Z

Should the new property set by the move method be hoverTarget or _hoverTarget (and should it be added to the list of properties that are blocked from being SET in routines)?

I would not add the underscore prefix. I don’t think this is virtual in the same sense that the true virtual properties are.

As for being able to SET it, I can’t think of when I would want to, but why prohibit it? I can’t think of a reason for that either. So just leave max flexibility for game designer.

96LawDawg · 2022-10-25T19:07:07Z

On second thought I can think of a use case for SETting the property. Maybe I want to turn it off if x>1000, so allowing me to SET it would allow me to override the normal behavior I want if x<1000.

flackr · 2022-10-26T03:17:59Z

Another question is should the cursor also disappear when this property is set - since it has similar semantics as not technically a child but being dragged over a hidden widget. I think this could probably be done after as a followup - but just throwing the idea out there.

flackr · 2022-10-26T03:22:57Z

On second thought I can think of a use case for SETting the property. Maybe I want to turn it off if x>1000, so allowing me to SET it would allow me to override the normal behavior I want if x<1000.

I'm not sure I follow the use case. You want to replace the hoverTarget if x>1000? Would you expect the dragged item to drop into the replaced hoverTarget if released? If so, I'd need to replace the reference to this.hoverTarget in moveEnd with this.get('hoverTarget'). You'd probably need to observe changes to the property to ensure that you keep overriding it as the item is dragged over new targets.

96LawDawg · 2022-10-26T03:31:05Z

Another question is should the cursor also disappear when this property is set - since it has similar semantics as not technically a child but being dragged over a hidden widget. I think this could probably be done after as a followup - but just throwing the idea out there.

I don't think you should hide the cursor. That would most likely cause confusion.

96LawDawg · 2022-10-26T03:35:54Z

I'm not sure I follow the use case. You want to replace the hoverTarget if x>1000? Would you expect the dragged item to drop into the replaced hoverTarget if released? If so, I'd need to replace the reference to this.hoverTarget in moveEnd with this.get('hoverTarget'). You'd probably need to observe changes to the property to ensure that you keep overriding it as the item is dragged over new targets.

Well, it's not a real use case because I don't really know how I would use it in a game. But being able to SET a property (rather than putting it on a list of properties that cannot be SET) gives me more options as a game designer. Just for example of how it might work, look at https://test.virtualtabletop.io/PR-1357/lawdawg. Watch the JSON of a card change over the hand as the x coord gets to 1000. That might allow me to do something. So leave it as is.

And, no, don't cause that to drop. That would be the point of overwriting it is to cause it NOT to drop for some reason.

flackr · 2022-10-26T03:38:55Z

I don't think you should hide the cursor. That would most likely cause confusion.

My thinking here is if someone's interacting with content I can't see, it's more confusing to see their cursor moving around over the things that I see in that place instead. Also, it would hide "private" information. However, if it's contentious I'll open a separate issue for discussion.

96LawDawg · 2022-10-26T03:48:40Z

I don't think you should hide the cursor. That would most likely cause confusion.

My thinking here is if someone's interacting with content I can't see, it's more confusing to see their cursor moving around over the things that I see in that place instead. Also, it would hide "private" information. However, if it's contentious I'll open a separate issue for discussion.

Oh. You mean hide other players' cursors. That would be a nice option to have, but I wouldn't make it the default. That was one thing I was trying to do with my demo room, but it is tricky applying the CSS needed to just one player. But if it was built into the code and there was a property or parameter for turning it on and off, that would be sweet.

flackr · 2022-10-30T04:28:24Z

I'm finding that I'd really like to have dragged pieces also apply the scale of the container they're dragged over so you don't get the effect of pieces on scaled holders changing size when you pick them up and again when you drop them. Do you think this would be reasonable to add to the same property, and call it something like hoverAppliesChildrenEffects to cover scale and visibility? Or would you rather see this as a separate property?

96LawDawg · 2022-10-30T13:59:36Z

I'm finding that I'd really like to have dragged pieces also apply the scale of the container they're dragged over so you don't get the effect of pieces on scaled holders changing size when you pick them up and again when you drop them. Do you think this would be reasonable to add to the same property, and call it something like hoverAppliesChildrenEffects to cover scale and visibility? Or would you rather see this as a separate property?

That would be a really nice feature. I don't think it is related in any way to onlyVisibleForSeat. It would be nice to be able to do this with any widgets that you start to drag. Keeping the scale (and maybe optionally even rotation) of the original parent when you start to drag would be very useful.

But that is probably a different paramater/property or whatever. I can't keep all those terms straight.

flackr · 2022-10-30T14:12:36Z

But that is probably a different paramater/property or whatever. I can't keep all those terms straight.

Sounds good, I opened a new issue for it: #1379

I think this PR is ready to land given the decisions we made about it needing a property and not rewriting games yet, just needs a code review.

flackr · 2022-11-01T01:17:15Z

I wonder if we should replace children in the property name with hover. I think it might make more sense since technically children already implicitly are affected by ancestor visibility where hovered widgets are not.

robartsd · 2022-11-02T15:28:42Z

I wonder if we should replace children in the property name with hover. I think it might make more sense since technically children already implicitly are affected by ancestor visibility where hovered widgets are not.

Yes, I like hoverInheritVisibleForSeat better as a new property name for this.

robartsd · 2022-11-02T18:21:46Z

Perhaps hoverInheritChildrenPerOwner could do similar for hands that are not associated with seats (not necessarily in this PR).

robartsd · 2022-11-02T18:36:16Z

Updates to the Global Widget Properties table in the wiki:

Property	Default value	Description
dragging	null	The playerName of the player currently dragging the widget. Used internally along with `hoverTarget` to implement behavior of `hoverInheritVisibleForSeat`; not intended to be set by game.
hoverInheritVisibleForSeat	false	Widgets dragged over the hover area of a valid `dropTarget` implement the most restrictive `onlyVisibleForSeat` property of any ancestor with this property set to true. This creates an effect similar to the `childrenPerOwner` property of holders.
hoverTarget	null	The `id` of the widget that will become the parent of a widget being dragged if it is dropped. Used internally along with `dragging` property to implement behavior of `hoverInheritVisibleForSeat`; not intended to be set by game.

flackr · 2022-11-02T23:12:57Z

Perhaps hoverInheritChildrenPerOwner could do similar for hands that are not associated with seats (not necessarily in this PR).

Yep, can do this. I'm planning to also reuse the added hoverTarget property to support hover inherited scale so that authors can avoid the size change you see when you pick up and drop items over scaled holders (#1379).

server/fileupdater.mjs

96LawDawg · 2022-11-08T19:02:23Z

How close do we think this PR is to being ready? This is the thing I needed for so many games that I didn't know I needed until now. Now I have a game that I want to make using this. I know I could make it in the PR test rooms, but I'm hoping this will be ready soon.

client/js/widgets/widget.js

flackr · 2022-11-08T19:21:52Z

I believe this is just waiting on a code review. I think we've thrown around enough ideas for alternatives that the property changes we have for this now is reasonable. We've also updated this to be the default with fileupdater for older games. @ArnoldSmith86 have you had a chance to look at the code change?

ArnoldSmith86 · 2022-11-08T21:30:34Z

I know I could make it in the PR test rooms, but I'm hoping this will be ready soon.

It will be ready sooner if you test it by making games in the PR test rooms.

Add property to inherit onlyVisibleForSeat while dragging.

4470765

While dragging, inherit the onlyVisibleForSeat property from the current hover target to hide visibility for changes made over a hidden widget.

flackr requested a review from ArnoldSmith86 as a code owner October 14, 2022 03:06

Inherit onlyVisibleForSeat from ancestors.

2670ecc

ArnoldSmith86 added enhancement New feature or request widget properties changes to widget properties labels Oct 14, 2022

Sync hoverTarget and dynamically compute visibility on clients.

22b127d

flackr added 2 commits October 14, 2022 17:01

Add hoverTarget to property list.

5f53d26

Default hoverTarget to null

42727a6

robartsd reviewed Oct 14, 2022

View reviewed changes

client/js/widgets/widget.js Outdated Show resolved Hide resolved

Move added method to sorted location.

cc34775

96LawDawg approved these changes Oct 15, 2022

View reviewed changes

Merge branch 'main' into drag-only-visible-to-seat

efd6005

robartsd mentioned this pull request Oct 25, 2022

Add ability to explicitly use a widget property as a string in CSS #1373

Open

Merge branch 'main' into drag-only-visible-to-seat

161da56

Rename property to hoverInheritVisibleForSeat

8ad49a9

Update default to true and use fileupdater to preserve old behavior.

b39e88f

robartsd reviewed Nov 5, 2022

View reviewed changes

server/fileupdater.mjs Show resolved Hide resolved

Merge branch 'main' into drag-only-visible-to-seat

feb42bc

robartsd reviewed Nov 8, 2022

View reviewed changes

client/js/widgets/widget.js Show resolved Hide resolved

Avoid trying to access invalid parent.

17e5309

robartsd approved these changes Nov 8, 2022

View reviewed changes

Merge branch 'main' into drag-only-visible-to-seat

e7b1d6c

flackr merged commit 9148e5a into ArnoldSmith86:main Nov 8, 2022

Add property to inherit onlyVisibleForSeat while dragging. #1357

Add property to inherit onlyVisibleForSeat while dragging. #1357

Conversation

flackr commented Oct 14, 2022

ArnoldSmith86 commented Oct 14, 2022

flackr commented Oct 14, 2022 • edited Loading

flackr commented Oct 14, 2022

96LawDawg commented Oct 14, 2022

96LawDawg commented Oct 14, 2022 • edited Loading

flackr commented Oct 14, 2022

96LawDawg commented Oct 14, 2022 • edited Loading

flackr commented Oct 14, 2022

96LawDawg commented Oct 14, 2022

flackr commented Oct 14, 2022

ArnoldSmith86 commented Oct 14, 2022

flackr commented Oct 14, 2022 • edited Loading

ArnoldSmith86 commented Oct 14, 2022

robartsd commented Oct 14, 2022 • edited Loading

flackr commented Oct 14, 2022

robartsd commented Oct 14, 2022 • edited Loading

flackr commented Oct 14, 2022

96LawDawg left a comment • edited Loading

Choose a reason for hiding this comment

flackr commented Oct 15, 2022

96LawDawg commented Oct 25, 2022

96LawDawg commented Oct 25, 2022

flackr commented Oct 26, 2022

flackr commented Oct 26, 2022

96LawDawg commented Oct 26, 2022

96LawDawg commented Oct 26, 2022 • edited Loading

flackr commented Oct 26, 2022

96LawDawg commented Oct 26, 2022

flackr commented Oct 30, 2022

96LawDawg commented Oct 30, 2022 • edited Loading

flackr commented Oct 30, 2022

flackr commented Nov 1, 2022

robartsd commented Nov 2, 2022

robartsd commented Nov 2, 2022 • edited Loading

robartsd commented Nov 2, 2022 • edited Loading

flackr commented Nov 2, 2022

96LawDawg commented Nov 8, 2022

flackr commented Nov 8, 2022

ArnoldSmith86 commented Nov 8, 2022

flackr commented Oct 14, 2022 •

edited

Loading

96LawDawg commented Oct 14, 2022 •

edited

Loading

96LawDawg commented Oct 14, 2022 •

edited

Loading

flackr commented Oct 14, 2022 •

edited

Loading

robartsd commented Oct 14, 2022 •

edited

Loading

robartsd commented Oct 14, 2022 •

edited

Loading

96LawDawg left a comment •

edited

Loading

96LawDawg commented Oct 26, 2022 •

edited

Loading

96LawDawg commented Oct 30, 2022 •

edited

Loading

robartsd commented Nov 2, 2022 •

edited

Loading

robartsd commented Nov 2, 2022 •

edited

Loading