item.roll() does not preserve the item in most cases, causing counter-intuitive results and errors #1049

Open aaclayton opened 3 years ago

Originally in GitLab by @caewok

Setup

Foundry 0.7.9, dnd5e 1.2.4, no modules enabled

Server: Docker on a Vultr 1 gig cloud server. https://github.com/felddy/foundryvtt-docker (shouldn't matter for this issue)

Browser: Firefox 86.0.1 on Mac OS X 10.15.7 (also shouldn't matter for this issue)

Issue

When rolling an item such as a weapon or a spell, the resulting chat card usually links back to the item in the actor, instead of storing a copy of the item. (The exceptions I can see are (1) if the item is consumed on roll; and (2) if spell level is increased, spell slot level increases are preserved but not the rest of the item rolled.)

This causes at least three problems:

If the item is rolled, but then subsequently changed in the actor, clicking on the attack/damage for the item pulls the changed item data and not the original. Reasonable minds can differ on this, but I view this as a counterintuitive and unwanted result. In my view, once an actor uses an item and the card shows up in chat, the expectation is that the use is frozen in time based on the item at that time. If I wanted to modify the weapon or spell after the initial roll, I would expect to have to re-roll with the new item. Accessing the previous roll in the chat should take me back to the item as it was when it was rolled and displayed into chat. (Compare to dragging the item to the macro bar, where I would expect the macro to access the current version of the item, acting like a shortcut for rolling the item.)
If the item is rolled and then subsequently deleted in the actor, the chat buttons for that roll no longer work and throw an error, because the actor's item is not found. This is a more extreme version of (1).
Modifying a weapon or spell in a macro and then rolling using that temporary item does not work, because the resulting chat message buttons will instead look up the original version of the item. This is particularly problematic for certain use cases, such as using a macro to allow the user to temporarily change the damage type or other aspects of a spell. (See sorcerer metamagic for some examples of why one might want to do this.)

Details

displayCard in entity.js only embeds the item rolled if it is consumable and the actor no longer owns the item:

// If the Item was destroyed in the process of displaying its card - embed the item data in the chat message
    if ( (this.data.type === "consumable") && !this.actor.items.has(this.id) ) {
      chatData.flags["dnd5e.itemData"] = this.data;
    }

While this is a correct check, it is not the only situation in which one might want to embed the item. Following that, clicking attack, damage, or create template buttons in the resulting chat message for the rolled item results in a call to _onChatCardAction in entity.js. _onChatCardAction looks up the stored data flag, and if the item is not stored, pulls the current version of the item from the actor using the item id. Spell level is handled as a unique case:

// Get the Item from stored flag data or by the item ID on the Actor
    const storedData = message.getFlag("dnd5e", "itemData");
    const item = storedData ? this.createOwned(storedData, actor) : actor.getOwnedItem(card.dataset.itemId);
    if ( !item ) {
      return ui.notifications.error(game.i18n.format("DND5E.ActionWarningNoItem", {item: card.dataset.itemId, name: actor.name}))
    }
    const spellLevel = parseInt(card.dataset.spellLevel) || null;

Proposed Solution

The easy fix for Issue (3) would be to add a way for item.roll() to pass through a storeData flag that causes displayCard to store the data. Basically, we just need another check along with the consumable item check that causes displayCard to set the "dnd5e.itemData" flag. This could be done using a flag in the item itself, such as a temporary flag that tells the subsequent functions to store it and not rely on the actor version of the item.

A relatively easy fix for Issue (2) would be to always store the item data in chat and in _onChatCardAction, first check if the item still exists in the actor and fall back on the stored data if the item is no longer found. This would obviate the need for a consumable item check in displayCard.

I believe that to fix Issue (1) would require always storing the item data and never pulling it from the actor after the chat message has been created. This would be a user-facing change from past practice, so if you agree that it is the correct result, it should probably be done on a major version change with some notice of the change.

Presumably, storing item data in chat is not resource-intensive because the item data will typically be linked to a single item and the chat can be deleted or archived as necessary.

Replication

For weapons, simply roll a weapon so that attack and damage buttons appear in the chat. For Issue (1), after the roll, change the weapon damage and then click the damage button to see that it the damage is now changed. For Issue (2), after the roll, delete the weapon from the actor and then click attack or damage buttons in the chat.

For spells and Issue 3, the following macro might help show one use case. To use, select a token that has Burning Hands. Note that I have tried two ways to create the upgraded temporary Burning Hands spell, but both ways fail to work properly with the resulting chat buttons.

(async() => {
// Select a token that has burning hands 
if(canvas.tokens.controlled.length == 0) {
  return ui.notifications.error("Must select a token!");
}

let token_selected = canvas.tokens.controlled[0];
console.log("Token selected:", token_selected);

const spell_name = "Burning Hands";
let spell_chosen = token_selected.actor.data.items.filter(item => "spell" === item.type && spell_name === item.name);

if(spell_chosen.length == 0) {
  return ui.notifications.error(`Selected actor does not have ${spell_name}!`);
}

spell_chosen = token_selected.actor.getOwnedItem(spell_chosen[0]._id);
console.log("Spell chosen:", spell_chosen);

// switch out the damage type 
const chosen_dmg_type = "cold";
let dmg_parts = duplicate(spell_chosen.data.data.damage.parts);
dmg_parts[0][1] = chosen_dmg_type;

// double the template size
const spell_target_size = spell_chosen.data.data.target.value;

const spell_modifications = { "data.damage.parts": dmg_parts ,
                              "data.target.value": spell_target_size * 2,
                              "data.components.somatic": false,
                              "data.components.vocal": false };

const upcastData = mergeObject(spell_chosen.data, spell_modifications, 
                                 {inplace: false});

// two versions: either just use createOwned or first create a temporary item (which changes the id) and then use createOwned
// dnd5e uses createOwned when upcasting a spell
// this casts the spell but the resulting chat message damage uses the original unmodified spell (test by clicking place template; it will revert back to 15-feet)
// const updated_spell_to_cast = spell_chosen.constructor.createOwned(upcastData, 
//                                   token_selected.actor);

// createOwnedItem is async, unlike createOwned, and takes a temporary flag
// need the temporary flag to avoid creating the item in the actor
// this casts the spell but the resulting chat message buttons throw an error because the item is no longer found
const temp_item = await token_selected.actor.createOwnedItem(upcastData, {temporary: true});
const updated_spell_to_cast = spell_chosen.constructor.createOwned(temp_item, 
                                  token_selected.actor);

console.log("Updated spell:", updated_spell_to_cast);

// roll the spell using cold, not fire.                              
updated_spell_to_cast.roll();  

})();

Originally in GitLab by @caewok

mentioned in issue tposney/midi-qol#342

Originally in GitLab by @caewok

Right, so assume for the moment that you run some tests and there is enough of a resource impact to warrant something more complicated than just storing every item in its respective chat message.

In the short term, I think it is still worth adding a flag to the item indicating that the item is "temporary" and therefore must be attached to the chat message data. This is just an extension of the current practice of storing item data if the item is going to be consumed. Because the flag has to be enabled for the item to be stored in that manner, the resource impact is likely comparable to that of consumed items. This would make it much easier for macros/modules to manipulate certain item data in a one-off manner. For example, sorcerer metamagic, magic weapons that allow for occasional additional damage, or class features that give occasional modifications to attack/damage/weapon characteristics.

In the long term, consider approaching the item storage problem with something like this: Create a ChatDataCache class. The ChatDataCache takes in a unique id (likely the chat message id) and an object (here, the item to be stored). ChatDataCache will then either return the item or NOTFOUND if queried with the unique message id. ChatDataCache does not guarantee an item will be found, but operates simply to cache recently utilized items. The relevant chat message could then query the ChatDataCache instead of storing the full item data, thereby saving potentially significant resources.

Within the ChatDataCache class, store a time index and a hash. When given an object to save, hash the object, and associate the hash # with the time of entry and the provided unique message id. Thus, if the same item is repeatedly stored, it will have a matching hash and not take up additional resources. The time of entry permits one to track more recent entries. The time could be updated every time that hash is queried or stored. Then one can decide when and how to limit the cache. You could save only X most recent hashed items. Or you could store all items and trim when the GM logs out. This could limit the chances of a user encountering a NOTFOUND error by clicking on a prior chat message.

This type of ChatDataCache class may already exist in Javascript or in Foundry API; I am definitely no expert!

Originally in GitLab by @akrigline

I would have thought that each item with a unique id is stored separately in the database. So the chat data actually just links to the underlying unique item and so resources used would be minimal.

Either way, doesn't this plan create an extra copy of the Item in the database?

I'm not sure if there's much in the way of "timing out" data, but that's definitely a solution. It would lead to other potentially unexpected issues ("Any button I click in my chat history errors out!" vs "Any button whose item is missing...").

It would be interesting to profile some iterations of this and compare the impact. If you intend all chat buttons to snapshot their item's values for things like attack rolls and damage rolls that'll obviously have a higher impact than if you limit it only to chat cards which are allowed to pop out (as an arbitrary rule of thumb).

Originally in GitLab by @caewok

I would have thought that each item with a unique id is stored separately in the database. So the chat data actually just links to the underlying unique item and so resources used would be minimal. Maybe someone with more experience with the data storage part of Foundry can weigh in.

The impact may change if one were to repeatedly create unique temporary items, in which case I see several possibilities:

Don't care about it because the underlying data storage can handle it;
Drop the item data after a certain period of time has passed or a certain number of messages have been added to the chat after it; or
Store the delta of the temporary item changes against some ideal form of the item. (3) sounds like something that should be handled by the underling database and not by code at this level. Plus, what would be the ideal? The compendium version? Which compendium? You cannot use the actor's item, because to correct the problem we have to assume the actor's item may not exist when needed for an attack or damage roll.

In any event, a good short-term fix for at least part of this issue would be to accept a flag on the item that marks it as temporary and required to save in the chat. Presumably, most items would not enable the flag so impact on chat data should remain minimal.

Originally in GitLab by @caewok

mentioned in issue tposney/midi-qol#307

Originally in GitLab by @akrigline

Thank you for this super detailed issue regarding a potential change in how the system works!

Some more things to think about:

I think always storing the item in the chat data would be more resource heavy than is desired and would cause a lot of data bloat for relatively small gain.
This relates to #881 and solving this would probably solve that.

Spitballing: I don't know that it makes sense to store all of the item's data on the chat card, instead could only a subset of the item's data be stored? Would there be much gain in doing this from the data side?

foundryvtt / dnd5e