Functions in TokenPropertyFunctions.java to have mapName as optional parameter

[Merudo]

Is your feature request related to a problem? Please describe.
It is a pain to change properties of tokens on other maps. Instead of having to change the map and possibly create flickering, it would be better to add the mapName as additional optional argument.

Describe the solution you'd like
I would like the following functions in TokenImage.java to take a mapName parameter so they could work on tokens in other maps:

    getPropertyNames
    getPropertyNamesRaw
    hasProperty
    isNPC
    isPC
    setPC
    setNPC
    getLayer
    setLayer
    getSize
    setSize
    resetSize
    getOwners
    isOwnedByAll
    isOwner
    resetProperty
    getProperty
    setProperty
    isPropertyEmpty
    sendToBack
    bringToFront
    setPropertyType
    getPropertyType
    getRawProperty
    getTokenFacing
    setTokenFacing
    removeTokenFacing
    getTokenRotation
    getMatchingProperties
    isSnapToGrid
    setOwner
    setOwnedByAll
    getTokenNativeWidth
    getTokenNativeHeight
    getTokenWidth
    getTokenHeight
    setTokenWidth
    setTokenHeight
    getTokenShape
    setTokenShape
    getGMNotes
    setGMNotes
    getNotes
    setNotes
    setTokenSnapToGrid

[Azhrei]

I presume the fix for propagating tokens to other clients was solved by adding putToken(), but there are performance ramifications to adding it everywhere, which is why it hadn't already been done.

Every call to putToken() causes the entire token to be sent (which includes all properties and macros), and if you call putToken() twice, the token is sent twice. There is no "queue" where attempts to send the token multiple times are collapsed into sending just the last one.

I don't think it's a good idea to add putToken() to all of these — imagine a macro that makes changes to a lot of properties all at once!

However, maybe there should be a function that could push named tokens to clients under macro control? This new refreshClientTokens() macro (or whatever it's called) could take a list of named tokens and call putToken() on each one. Note that putToken() isn't really the right function to call — it should really call a lower-level function that just doesn't the client update, but such a function doesn't exist. The entire putToken() call graph needs to be refactored; that could be a fair amount of work. I looked at it some time ago but I don't remember the details.

[JamzTheMan]

I tried to look at that a little bit ago but didn't have time for that rabbit whole (and agree). I suspect setState has putToken somewhere? My macros are horribly performing due to the token copies.

I thought maybe a macro to deferPutToken of some sort? Similar to your idea but instead call that this function at the start of your macro chain and set a boolean, put token is then only done once and only after all macros are done. (How ever that magic could be done, not sure if possible)

[Phergus]

Of course a lot of the set*() functions already had both a ServerCommand.putToken() and a Zone.putToken() call but not all. Some had one and some had the other. Which implies that some of the macro functions probably weren't propagating the changes out to the clients.

Note that the ServerCommand.putToken() does it's own call to Zone.putToken() so having both in the same macro is probably a bad thing. But, the ServerCommand.putToken() method is calling Zone.putToken() on the current zone which is going to be a problem since the goal of this change is to update a token on a zone (i.e. map) that isn't the current one.

[Merudo]

Phergus: thanks for the observation that ServerCommand.putToken() does its own Zone.putToken(). I suggest all duplicated Zone.putToken() be removed (#549).

By the way, ServerCommand.putToken() takes zoneGUID as argument, so it can update a token on any zone.

[Merudo]

Also, is the performance issue with Zone.putToken(), or is it with ServerCommand.putToken() specifically?

My understanding is that Zone.putToken() updates the token locally, while ServerCommand.putToken() updates it both locally and on other clients.

I assume there would be a performance gain for calling Zone.putToken() instead of ServerCommand.putToken()?

[Merudo]

My problem with the lack of update is that if another client interacts with the token right away, the changes get canceled.

As for the general debate on the performance loss of ServerCommand.putToken(), I can see a few solutions:

1. Have some functions call putToken(), while some don't. It's not clear to me which functions should not call putToken(). In any case there should be some comments in the code justifying the lack of update. And as Azhrei said, a function refreshClientTokens() to push the update manually would be great.

2. Have a function, say, DisableUpdate(), disables all updates on the token until enabled back with EnableUpdate(). While the updates are disabled, refreshClientTokens() could be used to force the updates.

3. As JamzTheMan said, have a function deferPutToken() defers sending the updates until the macro is executed. I think it might be not as hard as anticipated. We could have deferPutToken() call EventQueue.invokeLater to defer putToken(). The code for execLink() could probably be reused.

4. Add a boolean argument to the macro functions, to decide if the changes should be pushed to clients or not.

I think option #3 with deferPutToken() has a lot of merit. Basically, deferPutToken() would set a boolean variable to 1, which would disable ServerCommand.putToken(). Additionally, deferPutToken() would add an event with EventQueue.invokeLater to update the tokens.

The big question I have is, where should that boolean variable be located? I suppose in the MapToolVariableResolver ?

[Azhrei]

This is going to be a tough problem to solve with the current design re: how tokens are updated. We’ve been down this rabbit hole before and I’d prefer to not change anything unless a comprehensive solution is presented, rather than a bandaid.

It may be that this should be tabled until the token split is achieved, since that will make token updates cleaner in the first place, and will likely necessitate some cleanup in this area in any case.

[Merudo]

That's fine Azhrei, but what should I do to my commit in the meanwhile? Should I just turn it back the way it was for now, with some functions not updating clients?

[JamzTheMan]

I'm on the fence for this. On one hand, It makes sense to hold off and properly design this with a split and differential updates (or what ever) and maybe a queue system (like rabbit mq or kafka) BUT....

I wouldn't also mind a band-aid either. If it greatly improved the clobbering of tokens and sped up state changes, It would help users over the next 6-12 months (AFAIK token split is NOT being worked on currently and such as, it could be over a year before we see anything on that front)

My 2 cents

[Azhrei]

And this is why I think putting it into the hands of the macro authors via a refreshClients() or similar macro function is the way to go. It's a short-term bandaid. In the future, we simply turn the function into a no-op and existing macros will continue to work fine. (Or maybe it gets defined as a subset of the functionality it would need right now.)

In terms of the two putTokens(), with one calling the other, I guess a summary of what is done in each one would be necessary the best way to write the bandaid. For example, is a json list of token IDs being passed to it? Or is the queue of tokens kept internally? Let's discuss it and see what comes out of the discussion...