Closed rikmarais closed 11 months ago
Hi @rikmarais, thanks for the suggestion.
I added the browse before the fetch for The Forge specifically as the path is redirected, but just retrieving the browse once on load does make more sense considering it should never change.
I'm not sure about caching the actual actor data as each one is different, but I'll have a think. There may be a way to cache the last however many to reduce the amount of retrievals.
Hey there 👋
Apologies for leaning into this issue again, but I wanted to report that I've found that an interaction with other modules such as Combat Booster and Monk's Combat Details which automatically select the next token during combats (sometimes more than once, triggering multiple browse
calls) will exacerbate the issue I reported before.
Unfortunately, we're seeing this sometimes cause a couple thousand unnecessary network requests over the course of a game session between all of the players, especially if it's combat heavy. Since this is such a popular module (~37% of Forge installs run it), it could have a non-negligible effect on server load, especially during peak times.
I'm not sure about caching the actual actor data as each one is different, but I'll have a think. There may be a way to cache the last however many to reduce the amount of retrievals.
That could work very well, I think. The trouble is the browse
which is done to check the .files
to see if the id.json
exists.
If the last X recently browsed id
s were cached along with their returned paths then the file could be fetched directly, which is a much less expensive operation. Or even the filenames array in the result from foundFolderPath.files
, timing the cache out every few mins, and if the currently browsed id
is in there (checked with endsWith
), fetching directly.
My initial plan was to just cache the path separately and then use that when a getData
call is made, however I wanted to try to find a way to avoid using FilePicker.browse
, especially as that only seems to return the actual path when at least one file exists.
From my own Forge account I can see that the files are stored in https://assets.forge-vtt.com/UNIQUEID/...
and I thought The Forge might have a function to determine and return this path easily. I asked on the Forge Support but it didn't get a response this time around and I've been busy so I haven't looked into it further as yet. If I can get this unique path, I could hopefully just assume it's correct and use it to build a path for the fetch.
Is it possible for the Forge to add something like ForgeVTT.assetLibraryUrl
for me to use?
Is it possible for the Forge to add something like
ForgeVTT.assetLibraryUrl
for me to use?
Certainly, yes! That's a good solution. You can get this with the below.
if (typeof ForgeVTT !== "undefined" && ForgeVTT.usingTheForge) {
const assetsLibraryUrl = ForgeVTT.ASSETS_LIBRARY_URL_PREFIX + await ForgeAPI.getUserId() + '/'
}
The getUserId()
API call to fetch the userId
is quick (and less expensive than the browse), but also a good candidate for caching.
It's specific to that client's configured Assets Library, but if the GM has shared access to their Assets Library with their players, then getUserId()
will give them the GM's userId, so there should be consistent behaviour with the module's current functionality if players are configured to be allowed to browse/upload.
Perfect, I'll go that route.
Fixed in Token Action HUD Core 1.4.19.
Describe the bug![image](https://github.com/Larkinabout/fvtt-token-action-hud-core/assets/34374883/4d338c78-d7b7-461e-a966-8d7ddb031a16)
DataHandler
'sGetData()
method does abrowse
on any individual token click to check if theactor
directory exists. This can be optimized, since theGetData
call happens very frequently during play. In any kind of network environment, there needs to be a fetch to the server, but it'll usually just return the same result.Perhaps this directory can be cached after the first fetch or created as part of createDirectories on init, since the folder is unlikely to change. Similarly, the individual
.json
settings fetched bygetData
might also be cached, making it unnecessary to repeat the fetch every time the token is selected.Though I can understand why you might not want to do it for the
.json
, I think the folder is a good candidate for caching.Steps to reproduce
Versions:
Additional context We're seeing large numbers of hits against
assets/browse
from certain games on The Forge, which eventually trigger rate limiting and can cause poor performance in a world. The browse call mentioned in this ticket is triggered by a manual process so it's unlikely to be the cause of rate limiting by itself, but we saw significant numbers of calls and tracked it back to this module, so we thought it best to notify you since this seems like a good candidate for caching :)For reference, the
browse
on select can be replicated on a local Foundry environment, it's not Forge specific behaviour, but the fact thatbrowse
is a network call on Forge compounds the lack of caching for path and relevant.json
for the actor a bit.