doItemRoll() incorrectly swapping out the item rolled

[tposney]

In GitLab by @caewok on Mar 24, 2021, 12:29

Foundry 0.7.9, dnd5e 1.2.4, midi-qol 0.3.92.

When using item.roll() in a macro, the underlying function, doItemRoll(), appears to be swapping out the item for the actor-owned version of the item for part of the execution. For example, if I use a macro to temporarily change Burning Hands damage from fire to cold, then midi-qol will (incorrectly) apply fire damage while the template animation (as applied by TokenMagicFX) will (correctly) apply the cold template. Similarly, changes to the spell template size, etc., do not work as expected.

I tracked the culprit down to line 311 of itemhandling.ts:
let baseItem = this.actor.getOwnedItem(this.id) ?? this;

By looking up the original item, this is causing the modified item "this" to be ignored. I am not sure why the item is being switched out at this point for the version owned by the actor. It seems unique to doItemRoll(); I don't see a similar check in doAttackRoll() or doDamageRoll(). Changing that line to simply let baseItem = this; solves the issue. But I am not sure what the check is meant to accomplish, so I am not sure if this change will work for other use cases.

Here is a sample macro if you would like to experiment. Just select a token that has burning hands, and run the macro.

// Select a token that has burning hands 
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)[0];
spell_chosen = token_selected.actor.getOwnedItem(spell_chosen._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});

const updated_spell_to_cast = await spell_chosen.constructor.createOwned(upcastData, 
                                  token_selected.actor);

console.log("Updated spell:", updated_spell_to_cast);
                
// roll the spell using cold, not fire. 
// but midi-qol still applies fire damage                                  
updated_spell_to_cast.roll();  

[tposney]

Hmm, it's a problem and I'm not sure of an obvious solution.

The reason that the item.roll makes a copy of the actor item, is that the item that is rolled is "modified" to have the spell level of the cast spell, so if the fireball spell is cast at level 8, item.level will be 8 for the item that is rolled, whereas we actually need the base item level (3 for fireball) to have damage scaling work. The core dnd5e code rolls the modified item, then throws it away, but by the time I get the item the initial item data has been lost and I need to go to the actor to get the item data.

I guess that the only data I am actually looking at is item level, I could simply grab the item level from the base level, but I've not really looked in any detail to make sure that would work.

[tposney]

I've had a look and I think I've find a pretty simple reorganisation of the code that lets the macro work.
Subject to testing, change should go out in next release.

[tposney]

In GitLab by @caewok on Mar 25, 2021, 04:19

Thanks! In case it helps anyone with similar issues, I came up with an alternative version of the macro that actually creates a temporary item. This works for the initial roll if the midi-qol workflow is fully automated. But the problem with the alternative version is that once the chat card is displayed, the temporary item is lost. This probably means that midi-qol workflows that are less automated would also fail, although I have not tested those combinations. See the dnd5e issue I opened here: https://gitlab.com/foundrynet/dnd5e/-/issues/1049.

(Replace the const updated_spell_to_cast line in my macro above with this version to see the difference.)

// 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);

[tposney]

fixed in 0.3.94. The dnd5e case (where the item no longer exists) has been a problem for some time I seem to remember. It used to surface when consuming a potion that was auto destroyed. I know that a fix for that was done but can't remember how it was dealt with, perhaps the item data is stored on the chat card.