search

DestinyItemManager / DIM

Destiny Item Manager
https://destinyitemmanager.com
MIT License
2.07k stars 643 forks source link

Transmog Ornaments' availability for plugging still needs fixing #7561

Closed robojumper closed 2 years ago

robojumper commented 2 years ago

DIM Version

Version 6.96.0.1001069 (beta), built on 26.12.2021, 16:42:03

Describe the bug

  1. Insert overload bow (or any other anti-champion gauntlets mod of your choice) into your currently active character's gauntlets.
  2. Attempt to insert overload bow into any other, currently unequipped gauntlets piece for that class.
  3. See that overload bow is unavailable.
  4. Create a loadout and attempt to add the overload bow armor mod to it.
  5. See that overload bow is missing.

This was reported in the Discord's #support channel with Thermoclastic Strike. Works fine on production DIM.

Diagnosis

The Bungie.net API has two main fields where plugs' availability is controlled: enabled and canInsert.

Prior to #7519, DIM checked enabled. Enabled is often true, even when the plug can't actually be inserted, for example for universal ornaments that weren't unlocked yet (reported in #7488). I changed it to canInsert in #7519 and thought I verified that this doesn't change behavior for other sockets, but missed this edge case.

The edge case here is that for these artifact mods canInsert is false and DIM interprets that as the mod being unavailable for the entire character, even though it's only the current item where it might not be insertable. Refactoring in #7546 retained the change:

https://github.com/DestinyItemManager/DIM/blob/ff57b9228f3250980f04b411ab30728d4ade032d/src/app/records/plugset-helpers.ts#L30-L33

Some code in DIM has comments that look to be related and has a workaround:

https://github.com/DestinyItemManager/DIM/blob/ff57b9228f3250980f04b411ab30728d4ade032d/src/app/item-popup/SocketDetails.tsx#L95-L103

Note that for insertFailIndexes the Bungie.net API docs say: "This list will be empty if the plug can be inserted." The list should be non-empty when canInsert is false, so plugIsInsertable is essentially a return true; -- at least I haven't hit a case where it returns false. But that's a separate issue.

It's probably best to revert from canInsert to enabled to fix the mods issue for now and reopen #7488 and try to find a better way to fix that.

bhollis commented 2 years ago

I reverted that logic. I'm not 100% sure how to correct the logic without plumbing item definitions into that function (and even then it's no sure thing)

robojumper commented 2 years ago

I checked what the D2 Companion App does and it shows all the ornaments and errors out with a Bungie.net error if you actually try to apply them, so this would need some manually determined rules after heuristically checking the plug item type. There's also a connection to #7465 for that "hand-rolling our canInsert rules" part.

bhollis commented 2 years ago

I thought I maybe had a lead because some of them have an insertionRule that's the empty string, so maybe we could detect it?

robojumper commented 2 years ago

At least Focusing Lens also has the empty string as its singular insertionRule's failureMessage even though you can't have two of them in the same item: https://data.destinysets.com/i/InventoryItem:3947616003