Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add loadout filters for total, weapon, and armor light level #10520

Merged
merged 4 commits into from
Jun 14, 2024

Conversation

dghost
Copy link
Contributor

@dghost dghost commented Jun 10, 2024

This might require some iteration before merge (or even being redone entirely) but I figured I'd put the code out there to start the conversation, at least.

Short version is this implements 3 new filters for the loadout search:

  • light:/power: which filters by the calculated light level of the loadout, if all slots are filled
  • weaponlight:/weaponpower: which filters by the calculated light level of the weapons, if all 3 slots are filled
  • armorlight:/armorpower: which filters by the calculated light level of the armor, if all 5 slots are filled

These liberally reuse the range syntax from the light: filter from the main inventory, so they largely have all the features you'd expect.

The "If all slots are filled" quirk comes from spending some time staring at the loadout UI - it only shows a light level for a loadout if there are items in all slots, which seems like a reasonable constraint so I carried it forward for all three filters. I could see an argument for weapon/armor light level filtering not needing this restriction, though.

As with the loadout UI (and inventory filters), artifact level is not taken into account when filtering.

My big concern here is that this is well outside my knowledge of the DIM codebase, so I kind of cobbled together the translation from a loadout -> DIM items + all the validation since I couldn't find any sort of pre-implemented utilities for it. I may have missed something very obvious, or honestly I may have simply done something that is counter to long term roadmap.

I'm also not sure I'm happy with having both the light: and power: variants, but it's no additional maintenance and it seemed worth it for symmetry.

Anyways, apologies for dropping this on you unannounced. I can ping you on discord if we need to chat about it some.

Copy link
Contributor

@bhollis bhollis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

docs/CHANGELOG.md Outdated Show resolved Hide resolved
src/app/search/loadouts/search-filters/freeform.ts Outdated Show resolved Hide resolved
src/app/search/loadouts/search-filters/freeform.ts Outdated Show resolved Hide resolved
src/app/search/loadouts/search-filters/freeform.ts Outdated Show resolved Hide resolved
src/app/search/loadouts/search-filters/freeform.ts Outdated Show resolved Hide resolved
src/app/search/loadouts/search-filters/freeform.ts Outdated Show resolved Hide resolved
src/app/search/loadouts/search-filters/freeform.ts Outdated Show resolved Hide resolved
selectedLoadoutsStore,
);

// The UI, at the time of implementing this, only shows the light level using the following rules:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should probably reuse the code the UI uses:

https://github.com/DestinyItemManager/DIM/blob/7aa6d1299e22ab0c2f8947dfa077fa64c0eedb51/src/app/loadout/LoadoutView.tsx#L217-L233

Which is called like:

let [items, warnitems] = getItemsFromLoadoutItems(
  itemCreationContext,
  loadoutItems,
  store.id,
  allItems,
  modsByBucket,
);
const loadoutItemsByCategory: Record<BucketSortType, ResolvedLoadoutItem[]> = Object.groupBy(
  items.concat(warnitems),
  (li) => li.item.bucket.sort ?? 'Unknown',
);
const power = loadoutPower(store, loadoutItemsByCategory);

- Remove armorlight, weaponlight filters
- Remove use of class
- Clean up manual filtering of items by their bucket hashes in favor of orderBy and bucket hash
- Some cleanup of types + resolving LoadoutItem's to DimItem's
@bhollis
Copy link
Contributor

bhollis commented Jun 13, 2024

Thanks for the update. I'm still wondering why not reuse the code from LoadoutView to calculate the power level, though.

@dghost
Copy link
Contributor Author

dghost commented Jun 13, 2024

Thanks for the update. I'm still wondering why not reuse the code from LoadoutView to calculate the power level, though.

Eep, sorry - got busy and haven't had a chance to follow up on that.

I had looked at this and largely agree - it's probably the right call. I think my biggest concern is that I couldn't figure out how to get to an itemCreationContext object from the limited state of the filter context. Since object creation is controlled enough to have it's own state I figure it's probably not a thing that I can simply make out of thin air, and it'll have to get passed in through the filter context somehow.

Think it's worth bringing up on the discord, since it's probably going to be non-trivial? Or am I overthinking it?

@bhollis
Copy link
Contributor

bhollis commented Jun 13, 2024

Ah, that's a good point. I think you'd need to dig into getItemsFromLoadoutItems a bit and try to extract the part that doesn't need an itemCreationContext. I think it's just there for creating fake "warnItems" which shouldn't count for power anyway.

@dghost
Copy link
Contributor Author

dghost commented Jun 14, 2024

Yeah, let me poke at it a bit. I should be able to pull the relevant bits out.

@dghost
Copy link
Contributor Author

dghost commented Jun 14, 2024

Alright, so I looked at it and the logic for getItemsFromLoadoutItems() is basically:

  for (const loadoutItem of loadoutItems) {
    const item = findItemForLoadout(useTheseDefs, allItems, storeId, loadoutItem);
    if (item) {
        // Apply socket mods
    } else {
        // Make warnItem
    }
  }

Which I'm guessing can be simplified down to just the call to findItemForLoadout() since (to my knowledge?) mods can't influence power level. I did look at making it return ResolvedLoadoutItem[] like getItemsFromLoadoutItems() but that seems a little overkill since it can't return objects that it can't find in inventory, and that's really the only extra information that ResolvedLoadoutItem adds.

If you'd like, I'd be happy to separate the logic in the (newly renamed) getEquippedItemsFromLoadout() so that the actual equipped item + armor/weapon/can be equipped filtering occurs outside it. Though at that point it would also be worth considering just inlining the function entirely since it's like... 10 or 12 lines in total? I'm flexible and defer to your preference + coding style.

Copy link
Contributor

@bhollis bhollis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great. Thanks for adding it!

@bhollis bhollis merged commit 7f4df36 into DestinyItemManager:master Jun 14, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants