[BUG] - [e5] - On Interact Macro doesn't populate items array into Macro execution context

[FeistyMango]

Have you tried to reset Item Piles' system settings? Yes

Describe the bug If I setup an On Interact macro to be executed from a particular Merchant, it is executed appropriately. However, the items that were either bought or sold do not appear in the context payload correctly. When buying, the item is just missing from the items array, when selling to a merchant, the items array is populated with an undefined.

To Reproduce Steps to reproduce the behavior:

1. Setup an NPC Merchant
2. Open the mechant's Item Pile Configuration
3. Under Main Settings Tab, attach a On Interact Macro using this code console.log(arguments);
4. Have a player character complete a Buy/Sell transaction with the merchant item pile
5. Review GM dev tools console and you'll see that the context object passed into the macro is missing the items information involved in the transaction

Expected behavior I should be able to receive a list of items that were involved in the transaction so I can use them in my macro script.

Screenshots When Selling

When buying:

Setup:

• Foundry Version: 10.291
• System Version: Dnd5e 2.1.4
• Item Piles Versio: 2.4.16
• Browser & version 110.0.5481.97 (Official Build) (64-bit)

Active modules:

• Item Piles
• libwrapper
• socketlib

[FeistyMango]

Confirmed the location of the bug, it appears either the bug is that the object was double wrapped unintentionally, or it was just improperly referenced in a mapping function right before it is emitted to the macro function

In the image: When selling to a merchant, this code path attempts to map the items using the line:

if (macroData.items) {
        macroData.items = macroData.items.map(item => targetActor.items.get(item._id));
      }

However, the object structure is item.item._id whereas it attempts to access item._id which results in the mapped array containing undefined values.

[FeistyMango]

The issue is slightly different when purchasing from a merchant. In that situation, the seller's item delta is empty, and that is the only actor that passes the _executeItemPileMacro()'s if (!PileUtilities.isValidItemPile(target)) return false; function. However, the PC item delta has the actual item in the list, but it does not pass this check, so the NPC's data is what is executed and sent to the Macro hook.

In this case, it seems like the bug is that the Merchant's item delta is not being populated.

[FeistyMango]

More info on the Buying from a Merchant scenario. I discovered it is because the logic doesn't take into account if the Merchant has been configured to have an infinite amount of an item. When the config is on, this bug manifests on the Purchasing scenario, when I disable the infinite quantity, then I start seeing the delta populated properly.

[Haxxer]

Thank you for the detailed write-ups. I will take a look and see what I can do about this.

[FeistyMango]

np, I was able to quickly make a fix quick for the section where the items are merged which was pretty simply. However, it seems like the idea here is that if you are not a ItemPiles actor, the intent was not to run the On Interact macro. So that means addressing the other issue upstream (since a short cut would be to just allow the PC to run the macro since they have the item in their delta properly, but that seems problematic since it would run the macro twice) in the logic that runs the Transaction and then finds the deltas of what was aded/removed. However, when you are an NPC with infinite inventory, there is going to be no database call where anything is actually removed, so it seems that the logic of calculating the delta upstream in the Transaction.prepare() is the appropriate place to fix the issue of the items not appearing as a delta? I haven't read through all the code after that which might be affected by the sudden appearance of the item so perhaps a metadata property indicating that it was sold, but not actually deleted whereever that may be necessary to take into account?

[Haxxer]

Yes, I was considering adding a new boolean flag to Transaction#appendItemChanges and Transaction#appendActorChanges, which would cause that specific call to not update/remove the item in question, but still set the delta. That way the delta would be returned at the end of the transaction, btu the updates or deletions have not occurred.

However, that seems somewhat deceptive to me; I would expect that the Transaction will be truthful in the data that it commits and returns. I think it would be better to construct a custom payment array to pass to the items and attributes portion of the macro execution, which contains all of the relevant item and attribute data of that trade. This makes more sense to me as the only time this really matter is when buying/selling from a merchant; altering the behavior of Transaction wholesale would be dangerous and still wouldn't be used in any other portion of Item Piles' API.

[Haxxer]

@FeistyMango

Contrary to what I just said, that was the easiest way to go about it without having to keep track of items before and after updates, deletions, and creations.

Would you mind trying out your implementation in the 2.4.17 alpha? Do back up your world, in case the alpha has any bugs: https://raw.githubusercontent.com/fantasycalendar/FoundryVTT-ItemPiles/next/module.json

[FeistyMango]

I do, I'll load it up locally and give it a go! 😆 I figured you would take umbrage with modifying the Transaction in such a way as I had a similar reaction to it as I typed out the suggestion. However, I also noticed that anything else was likely to be a harder effort to pull off.

[FeistyMango]

I haven't examined the code change you made but just testing it, it appears to fire off the macro twice. I presume once for each character.

Edit: Seems like this portion of the change is the likely culprit. Before it would pass through the function once for each actor and since the PC wouldn't be enabled as an item pile, it would break out early and not emit the function whereas it proceed on to run the macro for the NPC item pile merchant. However, that terniary operator seems like you run the same npc twice through the function now. Perhaps this should simplified to one call to it if I'm following the change correctly?

https://github.com/fantasycalendar/FoundryVTT-ItemPiles/compare/master...2.4.17#diff-48b40167546b7acbf0592235f1bcf89073b4949ea7ba9a3184dceb4c67399c73R2183

[Haxxer]

Ah yeah, that makes sense; I'll make the adjustment.

[Haxxer]

If I make a single macro call, we will have to decide how to handle which deltas to pass; we could potentially abandon that entire idea by simply using the prices property on this macro callback. Thoughts?

[FeistyMango]

For my particular purposes, I really want to have all that useful info about the items themselves as well as the prices. Couldn't we always emit based on the item deltas of the item pile npc? Yes, it will mean that an integration has to calculate whether it is a sale to or buy from transaction, but even just the prices themselves would be enough to determine that since Purchases from players would have negative numbers whereas sales to players would be positive numbers.

Edit: In essence, if you have the "list of items" from the NPC perspective, you also have the list from the PC perspective too right? So it seems like we can safely arbitrarily pick the NPC's delta.

[Haxxer]

Did you check out the prices properties? It has all of the information you are looking for, I assume.

[FeistyMango]

👁️ Ahhh I didn't even notice how that had been changed. Ya, that would work just fine for my use-case!

[Haxxer]

Here's what I'm thinking of changing:

const itemPileActorUuid = sellerIsMerchant ? sellerUuid : buyerUuid;

await this._executeItemPileMacro(itemPileActorUuid, {
  action: "tradeItems",
  source: sellerUuid,
  target: buyerUuid,
  sourceItems: sellerTransactionData.itemDeltas,
  sourceAttributes: sellerTransactionData.attributeDeltas,
  targetItems: buyerTransactionData.itemDeltas,
  targetAttributes: buyerTransactionData.attributeDeltas,
  prices: itemPrices,
  userId: userId,
  interactionId: interactionId
});

and

if (macroData.items) {
  macroData.items = macroData.items.map(item => targetActor.items.get(item?.item?._id ?? item._id));
}

if (macroData.sourceItems) {
  macroData.sourceItems = macroData.sourceItems.map(item => sourceActor.items.get(item?.item?._id ?? item._id));
}

if (macroData.targetItems) {
  macroData.targetItems = macroData.targetItems.map(item => targetActor.items.get(item?.item?._id ?? item._id));
}

[FeistyMango]

Sorry for the delay. So essentially, all deltas on a single event. I like that approach. It feels cleaner that the structure of the prices object approach. Also unified it into a single contract. Perhaps one other modification, knowing which side of the transaction the npc is on, would give this a very clean interface to work with. Perhaps convert the source and target to objects that include a piece of conditional metadata such as isNPC or isItemPileActor. That would allow the macro script writer to easily decide what type of behavior they want to execute depending on whether or not it was a sell to vs buy from scenario. In my use-case I’ve got a discord web hook that I tied into a macro whenever a transaction with the merchant occurs. And formatting the way that is output into discord is dependent on if it was a sell to or buy from situation.

[FeistyMango]

Whenever you have a chance to complete that change, I can run it through some testing again. Thanks for your responsiveness with this request!

[Haxxer]

Just updated the alpha, the macro data now includes everything, as well as a bool for sourceIsMerchant

[FeistyMango]

Awesome! I’ll be home and can test it in about 3-4 hours.

[FeistyMango]

Performed some testing, see code review comments.

[FeistyMango]

Branched off your branch and added the missing loan. Put up a PR for it along with test script + screenshots

https://github.com/fantasycalendar/FoundryVTT-ItemPiles/pull/281

[Haxxer]

Thank you! I have merged this now, and shall push a release :)