RPTools / maptool

Virtual Tabletop for playing roleplaying games with remote players or face to face.
http://rptools.net
GNU Affero General Public License v3.0
798 stars 260 forks source link

[Bug]: Assets can be loaded from the asset cache without being part of the campaign. #4016

Open kwvanderlinde opened 1 year ago

kwvanderlinde commented 1 year ago

Describe the Bug

If a file is in the asset cache, I can use the MD5 key to display the image in HTML frames, set token images, etc. This works for a time, but if I ever clear the asset cache, send my campaign to a friend, or even try to broadcast an <img>, I don't actually know if things will continue to work or whether the big red X will rear its head.

To Reproduce

  1. Start a new campaign.
  2. Drag an image from the file explorer into the campaign. This should add it to the asset cache.
    • Note the MD5, e.g. via [r: getTokenImage('', getSelected())]
  3. Use the MD5 to show the image in chat. E.g., <img src="asset://md5-here...">
  4. Clear the asset cache.
  5. Close and reopen maptool, starting a new campaign.
  6. Try to use the MD5 in chat again.
  7. See the dreaded red X.

Expected Behaviour

In order to use an reference an asset, it should first be added to the campaign somehow. If an MD5 does not reference an asset in the campaign, the big red X should be shown.

Screenshots

image

MapTool Info

MapTool 1.12.2

Desktop

Linux Mint 21.1

Additional Context

As far as I know, this is how things have always worked. So whether this is really a bug or a (anti-)feature request, I don't know. I do know I'm not alone in having abused this in the past, but I don't actually like relying on it.

cwisniew commented 1 year ago

The standard answer has always been to include an asset in the campaign file if you wish to use it in that way. This is especially important for images on macro buttons, as the issue is much more significant than simply encountering a red X.

One potential solution to this problem could be to add an asset to the campaign the first time it is referenced, provided it is not already present in the campaign. However, this approach does have some drawbacks and does not fully resolve the issue, though it does come close.

Drawbacks include the fact that deleting an image from a campaign file is no longer a straightforward matter of checking if any maps, tables, states, or tokens are utilizing the image. Additionally, it is still possible for an image to be used but not fetched and not make it into the campaign file before sharing it.

While it is difficult to determine the ideal solution, it may be tempting to display a red X for any assets not in the campaign, even if they are in the cache. However, this approach likely would have unintended consequences.

FullBleed commented 1 year ago

Regardless, I feel like we need different kinds of x's to let people know what the issues are.

Btw, there is a hack where you set the File Sync Directory to the asset cache so you can use it to play locally stored audio files. I suspect that is not "supported" or "intended" behaviour (nor is it a widely used tactic), but since we don't have another way to serve local audio it is an option. So I hope any "fix" here doesn't break that given that there is nothing to replace the functionality.

cwisniew commented 1 year ago

Regardless, I feel like we need different kinds of x's to let people know what the issues are.

Btw, there is a hack where you set the File Sync Directory to the asset cache so you can use it to play locally stored audio files. I suspect that is not "supported" or "intended" behaviour (nor is it a widely used tactic), but since we don't have another way to serve local audio it is an option. So I hope any "fix" here doesn't break that given that there is nothing to replace the functionality.

I did say at the time that the above-mentioned hack would absolutely break when we get to a point where we reorganise the cache dir.

It is also this hack that I had in mind when I wrote, "...it may be tempting to display a red X for any assets not in the campaign, even if they are in the cache. However, this approach likely would have unintended consequences.." because replacing non-campaign assets with red (or yellow or stripped) X will break this.

So I think we have three options

It would seem that the status quo is the least objectionable answer.

FullBleed commented 1 year ago

Personally, I don't care if you break the hack to "fix" this issue... but it would be nice if you also gave us a directory that we could drop assets (Pdfs, video, audio, etc.) that could easily be served from local without being physically added to the campaign file. I'm sure there is probably a better way to do that than the file sync/asset cache "hack". The hoop jumping for the hack keeps the functionality out of reach of most users anyway.

cwisniew commented 1 year ago

Personally, I don't care if you break the hack to "fix" this issue... but it would be nice if you also gave us a directory that we could drop assets (Pdfs, video, audio, etc.) that could easily be served from local without being physically added to the campaign file. I'm sure there is probably a better way to do that than the file sync/asset cache "hack". The hoop jumping for the hack keeps the functionality out of reach of most users anyway.

That would just make this problem worse

kwvanderlinde commented 1 year ago

It would seem that the status quo is the least objectionable answer.

For the time being, I would agree. There are some legitimate use cases that can really only be accomplished via the asset cache for now. So I don't think we should close off this option until there are suitable features in place to replace it.

As far as FullBleed's suggestion of a directory, I think that's not far off the mark of solid feature. We already have the resource library which in some ways is similar in concept, i.e., as set of things readily available for use, yet not part of the campaign. If there was a way for a macro to temporarily reference files from there (some way that makes it clear it's not from the campaign), that would service some of the needs currently done by the asset cache. Especially if there were also a way to take that temporary reference and ask for the asset to be imported into the campaign, that would enable things like monster builders to bring in images as needed without having to preload a campaign with every possibility.

thelsing commented 1 year ago

I think before we add a way to handle files outside a campaign we should add ways to put them inside a campaign more easy. Like adding them to addon within maptool for example. I also think we don't have to break this "feature" without any good reason. But we also shouldn't add those files automatically to the campaign. Maybe we should log a warning and/or put a message of the missing asset in the chat?

FullBleed commented 1 year ago

But we also shouldn't add those files automatically to the campaign. Maybe we should log a warning and/or put a message of the missing asset in the chat?

I feel like non-campaign assets should be noted someplace more static. Like in a "folder" in the Library or Map Explorer. If the Map Explorer was called "Asset Explorer" or "Campaign Explorer" I'd definitely pick that one...

cwisniew commented 1 year ago

It would seem that the status quo is the least objectionable answer.

For the time being, I would agree. There are some legitimate use cases that can really only be accomplished via the asset cache for now. So I don't think we should close off this option until there are suitable features in place to replace it.

As far as FullBleed's suggestion of a directory, I think that's not far off the mark of solid feature. We already have the resource library which in some ways is similar in concept, i.e., as set of things readily available for use, yet not part of the campaign. If there was a way for a macro to temporarily reference files from there (some way that makes it clear it's not from the campaign), that would service some of the needs currently done by the asset cache. Especially if there were also a way to take that temporary reference and ask for the asset to be imported into the campaign, that would enable things like monster builders to bring in images as needed without having to preload a campaign with every possibility.

Isn't a location where you can have lots of assets and only pull in the ones you need exactly describing the resource library? Maybe there needs to be a way to include it that is not on a map, but I don't see the need for something else other than the resource library for this

kwvanderlinde commented 1 year ago

It would seem that the status quo is the least objectionable answer.

For the time being, I would agree. There are some legitimate use cases that can really only be accomplished via the asset cache for now. So I don't think we should close off this option until there are suitable features in place to replace it.

As far as FullBleed's suggestion of a directory, I think that's not far off the mark of solid feature. We already have the resource library which in some ways is similar in concept, i.e., as set of things readily available for use, yet not part of the campaign. If there was a way for a macro to temporarily reference files from there (some way that makes it clear it's not from the campaign), that would service some of the needs currently done by the asset cache. Especially if there were also a way to take that temporary reference and ask for the asset to be imported into the campaign, that would enable things like monster builders to bring in images as needed without having to preload a campaign with every possibility.

Isn't a location where you can have lots of assets and only pull in the ones you need exactly describing the resource library? Maybe there needs to be a way to include it that is not on a map, but I don't see the need for something else other than the resource library for this

That is exactly what I meant, no need for yet another thing.

FullBleed commented 1 year ago

Maybe there needs to be a way to include it that is not on a map, but I don't see the need for something else other than the resource library for this

That would be good.

So the questions becomes what the access methods would be:

Of course, any campaign that relies on those assets loses portability. But totally worth it to have a much easier method of access to massive local asset libraries (like bestiaries and music and large pdfs, etc.)

kwvanderlinde commented 1 year ago

Here's the idea that's been brewing in my mind lately ...

Just like asset:// URIs can identify a campaign asset, we could invent a new resource:// URI to identify resources that do not belong to the campaign. The structure of this URI is TBD.

These are the constraints I envision for resource URIs:

  1. Resource URIs never identify campaign assets
    • ... and ultimately asset URIs would no longer identify out-of-campaign assets.
  2. Resource URIs can be created for any file in the client's own resource library.
    • Means of creation TBD but I expect both manual interaction (e.g., dragging) and automated means (macro functions).
    • A player client cannot create resource URIs if Hide Players' Libraries is enabled.
    • Cannot create resource URIs for another client's resources (need to ask them to do it).
  3. Once created, a Resource URI can be communicated to and resolved on other clients.
    • To support things like playing a local audio file for all players.
    • We need to transfer resource files to clients when a resource URI is resolved (e.g., when used with playStream()), or perhaps more eagerly somehow.
    • In line with (2), should only be able to resolve from GM's client if Hide Players' Libraries is enabled.
  4. Resource URIs are ephemeral.
    • Because the available resources depends on the contents of each connected client's resource library, which can change at any time.
    • We can't stop people from storing Resource URIs, but we can discourage it by scoping it to a MapTool session or connection, or by some other means.
    • Ideally Resource URIs also wouldn't be hackable, or this point becomes moot.
  5. A Resource URI can be "converted" to an asset URI
    • Actual process would be importing the identified resource into the campaign as an asset.
    • Then the asset URL is determined as for any other asset.
    • Can't go the other way around.
cwisniew commented 1 year ago

I think it's better just to create a resources "explorer" and a way to add resources to the campaign that are not on the map (both er than add-ons as they already allow for this). Then resources URI could point to those resources. The resource URI could just be in the format resource://virtual/path/from/resource/explorer. It's not going to stop anyone from referring to resources that are not there/have been removed, but the above scenario isn't either. At least in this case if a GM refers to resource://images/chests/iron-chest and they have deleted the image so that it's not found, it should hopefully be obvious to the GM why it's not found and how to fix it (we can only do so much for them).

Oh and strongly discourage using "asset://" from that point onwards.

kwvanderlinde commented 1 year ago

I can't tell if you're suggesting that a resource file must be added to a campaign before it can be used in any capacity. I.e., whether a resource URI points to something in the campaign that was previously imported or something that only exists in the resource library. If the first option, then it does nothing to accommodate the current abuses of the asset cache.

cwisniew commented 1 year ago

I can't tell if you're suggesting that a resource file must be added to a campaign before it can be used in any capacity. I.e., whether a resource URI points to something in the campaign that was previously imported or something that only exists in the resource library. If the first option, then it does nothing to accommodate the current abuses of the asset cache.

Indeed I am suggesting it must be, and tell people to stop using asset... Because the plan for resource:// above doesn't really solve the problems, it does in some cases but there are sets of rules people need to follow to make sure they don't run into the same problem. So we just create two URI scheme with problems rather than solving the problem

kwvanderlinde commented 1 year ago

I'm still wondering what gets solved by your suggestion.

cwisniew commented 1 year ago

Neither solution solves the problem of people using assets in the campaign file that will not be available later when they try to use them in macros; you can tell people as much as you like that resources are ephemeral it won't stop them from putting it in a macro and trying to share a campaign with it, or having it fail when moving to a new PC, we have been telling people for more than a decade don't use asset:// for something that you haven't got in your campaign, and you see how well that has worked. Auto-adding them to the campaign on the first reference is also problematic as people will still put them in macros without doing a full test of all execution paths so stuff will get missed. Also since MT has no way to track references to them, it wouldn't know when it can remove them from the campaign file for those that are added.

But having resource storage (not tied to maps) gives people a place to store and refer to the assets. And by limiting it to paths in the campaign resource manager (for lack of a better term), they know they have to add it to the resource manager. If they don't add it, it will fail the first time they use it, so none of the "works for me" syndrome. Then we purge the wiki of all use of asset:// outside of add-ons (probably best if we can return resources instead of assets for add-ons too).

kwvanderlinde commented 1 year ago

Neither solution solves the problem of people using assets in the campaign file that will not be available later when they try to use them in macros;

That's always going to be impossible if we allow users to cook up anything and use it as a reference to an asset.

you can tell people as much as you like that resources are ephemeral it won't stop them from putting it in a macro and trying to share a campaign with it, or having it fail when moving to a new PC

It's not about uttering the term "ephemeral" and hoping people listen. If the same resource URI won't work the very next time the user opens MapTool, that will tend to discourage them away from trying to store them. Instead a new resource URI would be required each time the resource is needed to be used.

And it's not like the average user is going to be acquiring resource URIs. It's primarily library developers / macro authors that would encounter them, and (at the risk of generalize) they don't want to write things to just work once.

we have been telling people for more than a decade don't use asset:// for something that you haven't got in your campaign, and you see how well that has worked.

A few things there:

  1. It's the fault of the implementation for not constraining the lookup to the assets in the current campaign.
  2. Obviously people are going to try work around the things that limit their ability to use the program. Outside of the asset cache, they have exactly one option for using their own resource librarry: manually drag files from the library onto the map. The manually part sucks, and the bulk added ot the campaign file size sucks. Hence the abuse of the asset cache.
  3. It has nothing to do with the proposal. See above for why this isn't just saying "don't do unsupported things".

Using asset URLs that point outside the campaign isn't an accident, it takes effort to get or create them. If a user is unaware that they are using these asset URLs, it's not because they made a mistake and happened to not include the asset, it's likely because they're using a framework that intentionally abuses the asset cache and they weren't aware of that.

Auto-adding them to the campaign on the first reference is also problematic as people will still put them in macros without doing a full test of all execution paths so stuff will get missed.

This hasn't been suggested (though looking back I could have been clearer). Importing resources into a campaign would be explicit, just not necessarily manual.

A resource URI would not be usable everywhere that an asset URL is. E.g., setTokenImage() would still require a campaign asset as the image needs to be saved to the campaing. But playStream() or HTML5 <img> tags could use a resource URI directly, akin to how public URLs can be used without first needing to import them into the campaign.

If we wanted to use a resource URI to set a token image (or a variety of other things), we would first have to import the resource (can be a function to do that) and then can use the resulting asset URL.

But having resource storage (not tied to maps) gives people a place to store and refer to the assets. And by limiting it to paths in the campaign resource manager (for lack of a better term), they know they have to add it to the resource manager. If they don't add it, it will fail the first time they use it, so none of the "works for me" syndrome. Then we purge the wiki of all use of asset:// outside of add-ons (probably best if we can return resources instead of assets for add-ons too).

I won't complain about having a nicer way to manage campaign assets, especially something detached from maps. But this is really beside the point and does nothing to address the underlying issue: that users who want convenient access to their available resources either have to abuse the unreliable asset cache, or have to go through the hassle of bulking up their campaign file with all the possibilities beforehand.

I also don't understand this idea of essentially deprecating asset:// URLs in favour of resource:// URIs. If resources need to be added to the campaign anyway, the two would be functionally the same. Well, aside from the unsupported behaviour of asset:// URLs directly referencing the asset cache, but we all want that to go away eventually ... right?

cwisniew commented 1 year ago

Neither solution solves the problem of people using assets in the campaign file that will not be available later when they try to use them in macros;

That's always going to be impossible if we allow users to cook up anything and use it as a reference to an asset.

you can tell people as much as you like that resources are ephemeral it won't stop them from putting it in a macro and trying to share a campaign with it, or having it fail when moving to a new PC

It's not about uttering the term "ephemeral" and hoping people listen. If the same resource URI won't work the very next time the user opens MapTool, that will tend to discourage them away from trying to store them. Instead a new resource URI would be required each time the resource is needed to be used.

And it's not like the average user is going to be acquiring resource URIs. It's primarily library developers / macro authors that would encounter them, and (at the risk of generalize) they don't want to write things to just work once.

If a new resource URI is required for every use then I am not seeing how this is targeted to library developers, it seems more targeted to end users. I then fear it would lead to bug reports "resource URI fails the second time". Or am I missing something in how its intended to be used?

A resource URI would not be usable everywhere that an asset URL is. E.g., setTokenImage() would still require a campaign asset as the image needs to be saved to the campaing. But playStream() or HTML5 <img> tags could use a resource URI directly, akin to how public URLs can be used without first needing to import them into the campaign.

I think this is part of the confusion as you have suggested it as a solution to the problems in the issue but it doesn't solve the problems in the issue it addresses a separate issue altogether. If its just to solve things like playing music streams its better not as a URI at all as any URI scheme we introduce is usable through out and will be abused.

I also don't understand this idea of essentially deprecating asset:// URLs in favour of resource:// URIs. If resources need to be added to the campaign anyway, the two would be functionally the same. Well, aside from the unsupported behaviour of asset:// URLs directly referencing the asset cache, but we all want that to go away eventually ... right?

I am addressing the crux of the issue as logged, things need to be added to the campaign to be useable, people are abusing asset://, I have already been warned people will complain if I stop asset:// for accessing things not in the campaign (although I am still very tempted to do so), so the best solution to the raised issue that I can see is force them to add it to a campaign resource manager because they have to use the path from the campaign resource manager to access it, not the MD5key from the cache.

Having one-shot resource:// URI would be trying to address a different issue altogether, and I am not sure how it would properly address that. I am not saying it won't just that I am unable to see how, maybe I need an ELI5 to understand.