Disallow vendorItems without vendorHash from showing up in compareCategoryItemsSelector

[FlaminSarge]

Fixes #10279

When an item being considered for Compare has a vendor but does NOT have a vendorHash, it is DQ'd. Items from allItems won't have vendors (since they're player inventory items) and items in vendorItems should all have vendors, but the invalid ones will have vendorHash=0.

This is the most direct way to prevent the linked issue, solely affecting Compare view without potentially having side effects on any other view.

The alternative solutions for this involved making a change here: https://github.com/DestinyItemManager/DIM/blob/master/src/app/vendors/selectors.ts#L122
either by adding vs.component?.canPurchase or !vs.def.inhibitBuying checks inside a filter.
I did not know how the alternative solution would affect things other than Compare. I also wasn't too clear on how canPurchase and inhibitBuying differed, either, so I didn't go for the alternative solution.

Of note that loadout-builder-vendors.ts's selection already enforces a particular set of allowed vendors, but it could probably make use of a check such as canPurchase or inhibitBuying instead of that.

[bhollis]

I'm not familiar with these vendors - what are they? Should we be building vendor items for items without a valid vendor at all?

[FlaminSarge]

I'm not familiar with these vendors - what are they? Should we be building vendor items for items without a valid vendor at all?

Example: Defiant Armor https://data.destinysets.com/i/Vendor:4264351379
Used for the 'Preview contents' on the rep reward entries for the War Table's engram reward.

More details in #10262's comments.
I'm under the impression that we still need to build the vendor's items for the 'Preview contents' feature to work correctly, but we should not include those items in Compare views.

[bhollis]

Makes sense, though it does seem like those should have a vendorHash still (clearly, we know what vendor they're from). My worry would be if we fixed that root issue, they'd show up in Compare again.

[FlaminSarge]

Makes sense, though it does seem like those should have a vendorHash still (clearly, we know what vendor they're from). My worry would be if we fixed that root issue, they'd show up in Compare again.

Will we ever set vendorHash nonzero for vendorItemForDefinitionItem? (Is that a case where we'd want these no-stat items to show up in Compare so that they show which vendor they actually come from e.g. "Sold by: Defiant Armor" instead of "Sold by: --"?)

[bhollis]

Will we ever set vendorHash nonzero for vendorItemForDefinitionItem?

I'm not sure. I guess they don't need to have them to be displayed on the vendors page.

Is the problem on the Compare page that they have no vendor, or that they have no stats?

[FlaminSarge]

Will we ever set vendorHash nonzero for vendorItemForDefinitionItem?

I'm not sure. I guess they don't need to have them to be displayed on the vendors page.

Is the problem on the Compare page that they have no vendor, or that they have no stats?

The original issue was that they had no vendor and would outright crash DIM, I papered over that while investigating the root cause, but the issue linked here is to specifically exclude them from the Compare pane due to not having any useful stats to compare (I'd assume, @nev-r can chime in on it).
There's three questions that need answering for general DIM direction:

1. Do we want vendorItemForDefinitionItem to keep the vendor instead of discarding it? Doing so would make this particular vendorHash check moot.
2. Do we want these engram vendors to be considered for Compare view at all? There's some engram vendors with proper vendorHash, such as the second one here with vendor "Defiant Engram Decoding", where it obviously doesn't have stats, but at least for this case it tells you "here's where you can obtain this item".
   
   
3. Do we want "Compare" to sometimes not function at all when used on this view? If I click "Compare" here, because no other vendors are selling this item for my current character and I don't have any of that item in my inventory, Compare won't open at all if we outright exclude these items, which is confusing UX. Even my current patch probably runs into this issue, though I believe I can work around it by ensuring that the initialItemId is always included in the initial Compare view even if it's missing a vendorHash.
   

I've run into another fun bug that exists on Live while testing things for this: These no-stat items only have the itemCategoryHash for the class they're for, not the equipment slot, so categoryItems in compareCategoryItemsSelector gets everything for that class. This is true even for ones that have a valid vendorHash e.g. the ones in Zavala's Focused Decoding section.

[bhollis]

Do we want vendorItemForDefinitionItem to keep the vendor instead of discarding it?

Probably we do, though I can't think of anything that requires that.

Do we want these engram vendors to be considered for Compare view at all?

Probably yes, if the items have stats / perks. But not if they don't.

Do we want "Compare" to sometimes not function at all when used on this view?

No, it'd be best if those items were not "comparable".

These no-stat items only have the itemCategoryHash for the class they're for, not the equipment slot, so categoryItems in compareCategoryItemsSelector gets everything for that class.

That feels like a good thing to fix as well.

[FlaminSarge]

No, it'd be best if those items were not "comparable".

The problem with that is that if the user does have something similar to that item in their inventory, then the compare window should come up with those items, ignoring the dummy item that the 'Compare' originally came from. But we don't want the item tooltip to search through the user's inventory every time just to be able to figure out whether to show Compare or not...

Depending on how we fix this, the itemCategoryHash issue may end up moot, I guess.

[bhollis]

The problem with that is that if the user does have something similar to that item in their inventory, then the compare window should come up with those items, ignoring the dummy item that the 'Compare' originally came from.

Should it? Launching Compare from an item is meant to compare that item to other items, not to generically find similar items.

[FlaminSarge]

The problem with that is that if the user does have something similar to that item in their inventory, then the compare window should come up with those items, ignoring the dummy item that the 'Compare' originally came from.

Should it? Launching Compare from an item is meant to compare that item to other items, not to generically find similar items.

Sorry, by "similar to that item" I meant same copies of that item for the initial compare that they may have in their inventory or available as not-dummy items from other vendors, with the option to expand to e.g. other arm-slot items via the category selectors if they desire. Just excluding the no-stat items.
It wouldn't fulfill the purpose of "Compare this dummy item with real items I already own", but it would cover the case "Compare all items I own that are the same name/etc as this dummy item"

[bhollis]

Yup, I understand. I'm just calling out that I think it'd be OK for that to be out of scope for Compare.

[FlaminSarge]

Yup, I understand. I'm just calling out that I think it'd be OK for that to be out of scope for Compare.

...If we change the text on the button we could call it "See my copies" or something less verbose, and have an error/warning toast show up in the top right "Could not find any versions of that item in your inventory" instead of a no-op if the player doesn't have any other versions of that item. Thoughts? It's kind of out of scope of the linked issue but also a pretty elegant way to solve it with a new feature.

[bhollis]

I think that makes sense. The toast isn't so elegant but probably easier than an empty compare sheet.

[FlaminSarge]

Not sure if I should include this but session doesn't have any other info about the item attempting to be compared.

[FlaminSarge]

@bhollis does the use of session.query in the warning require that useEffect has session.query in useEffect's second parameter?

[bhollis]

@FlaminSarge it's not required, but it would be good to include it. If you include it, every time the query changes and there still aren't results, you'll get another notification.