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 breakermod to match weapons with seasonal artifact breaker mods #8914

Merged

Conversation

robojumper
Copy link
Member

@bhollis
Copy link
Contributor

bhollis commented Feb 13, 2023

Thinking we should probably close this one given the champion changes in Lightfall?

@robojumper
Copy link
Member Author

Yeah let's revisit this later.

@robojumper robojumper closed this Feb 13, 2023
@delphiactual delphiactual reopened this Jun 17, 2024
@delphiactual delphiactual marked this pull request as ready for review June 19, 2024 17:22
@delphiactual
Copy link
Contributor

@bhollis @robojumper updated and ready for review

const breakingIchs = breakerType.flatMap((ty) => artifactBreakerMods[ty] || []);
return (item) =>
Boolean(
!item.breakerType && item.itemCategoryHashes.some((ich) => breakingIchs.includes(ich)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this really be a new search, or should the breaker search just take into account unlocked artifact mods? What's the use case for finding items that are only granted breaker by the artifact?

And then we'd probably want to think about whether to show the breaker icon on the item tooltip.

Copy link
Contributor

@DHager DHager Aug 7, 2024

Choose a reason for hiding this comment

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

Should this really be a new search, or should the breaker search just take into account unlocked artifact mods?

I'm in favor of combining the logic into breaker, provided it handles cases like "Wish-Ender has intrinsic anti-barrier therefore it cannot get anti-overload from Overload Bow."

I can only think of one scenario where I might want to filter for the difference, and that's where I'm desperately trying to figure out if I can disable/avoid a champion mod and unlock something else without ruining a build... But in practice I tend to treat those seasonal mod choices as mandatory.

That said, it may be valuable to expose the breaker-"reason" as something that can be spot-checked on a particular item, such as an icon/tooltip indicating that Le Monarque has Anti-Overload intrinsically, while Hush is getting Anti-Overload from a seasonal mod.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, would you expect the breaker icon to also display on the item tile if an artifact mod could grant it breaker?

Copy link
Contributor

@DHager DHager Aug 7, 2024

Choose a reason for hiding this comment

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

would you expect the breaker icon to also display on the item tile if an artifact mod could grant it breaker?

Yeah, as someone who plays Too Damn Much, all the exotic weapons with breaker-status have permanent names/effects, distinct icons, and tend to get memorized and I don't even look for any icons. So showing permanent and seasonally-conditional results together is more helpful.

if (!breakerType) {
throw new Error(`Unknown breaker type ${breakerType as string}`);
}
const breakingIchs = breakerType.flatMap((ty) => artifactBreakerMods[ty] || []);
Copy link
Contributor

Choose a reason for hiding this comment

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

The tricky thing here is that not all of the artifact breaker mods may be active. Do we want to care about that? I assume most folks enable all the breaker mods, at least the basic ones, but then there's always some later column one that I ignore or haven't unlocked...

The tricky part of course being that each character has their own artifact.

Copy link
Contributor

@DHager DHager Aug 7, 2024

Choose a reason for hiding this comment

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

All the options have problems, but I find thinking "the principle of least surprise": Which model is easiest for users to recognize and understand?

To me, that's the one where the filter is showing consistent breaker potential via mods that may-or-may-not be unlocked yet, as opposed to "works on this character, but not on that character", or "worked yesterday, but not today", etc. In other words, filter results should only change when Bungie alters the seasonal-artifact.

@bhollis
Copy link
Contributor

bhollis commented Sep 3, 2024

Modified to extend the breaker: search instead of adding a new search, and to show the effective breaker type on the item popup and in the armory.

@bhollis bhollis merged commit 549be33 into DestinyItemManager:master Sep 3, 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.

4 participants