DestinyItemManager / DIM

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

Clean up DimPlug vs defs duality #9076

Open bhollis opened 1 year ago

bhollis commented 1 year ago

At some point the whole defs vs DimPlug duality needs to be cleaned up because we keep adding hacks specifically for subclass plugs where all we have is defs

Maybe one option would be to always convert a definition to a DimPlug?

see https://github.com/DestinyItemManager/DIM/pull/9070

robojumper commented 1 year ago

Here's the other hack I was thinking of when I mentioned this

https://github.com/DestinyItemManager/DIM/blob/e8b1a884a18aeecef380ba4b463e4cfb378c3c49/src/app/utils/plug-descriptions.ts#L121-L131

grafik

This should totally just say "-10 mobility" when you're on a Hunter, but it's difficult as long as the only thing we have is defs. And then sometimes the def-based plug drawer uses perks to show stat bonuses/penalties, some other def-based code uses stats, then then DimPlug-backed also uses both and stuff gets very annoying grafik

robojumper commented 1 year ago

Actually this is a good reminder; this may end up crashing DIM if Bungie removes this Charge Harvester CWL mod in Lightfall 😄

esotuvaka commented 1 year ago

Could we use an if statement / switch case here?

Possibly by using a map of possible subclasses and the corresponding class we can determine the appropriate stat that is getting lowered by "-10 to the stat that governs class ability recharge", since they'll be interacting with this loadout screen that is also containing the currently equipped subclass.

Using the subclass supers themselves would allow us to avoid passing state from higher up the component tree about the player's current class.

robojumper commented 1 year ago

Yes -- my point is that DIM already does this in some way. If you look at the tooltip in the last screenshot, the stats are correctly resolved already for a DimPlug. We even have a copy of that code for Defs because Loadout Optimizer needs it, but we don't use it for def-based UI. The point of this issue is to just consistently use what's already there for DimPlugs.

robojumper commented 1 year ago

Some more hacks in #9953 because we often look at the raw investmentStats from the definitions and DIM can't deal with multiple stats that provide values for the same stat hash:

robojumper commented 1 year ago

The reason fixing this is kind of complicated is that DimPlugs in DimPlugSets are in a "detached" state where no stats have been computed, and stats.ts:attachPlugStats sets the stats property on DimPlug ("attaches" the stats) when actually processing a concrete item, which we only do for plugOptions (which are the plugged plug and perk options, but not e.g. subclass config that comes from plugSets). In general it's impossible to statically figure out which concrete stat effects a plug will have once it's attached to an arbitrary item, since e.g. subclass fragments may affect a different stat per character class. Running attachPlugStats also for the entire plugSet can't be terribly performant and we might end up with a lot of objects. Maybe it won't be as bad with some fast paths and clone-on-write but it doesn't sound great.

Some thoughts: We can instead explicitly model "detached" DimPlugs and encode some rules about how their stats apply so that descriptions for detached plugs have an easier time figuring out what the exact amount is? Maybe we don't even need to attach the stats for not-plugged-plugOptions (unselected weapon perks, basically) and just keep them detached until we need the stats?

Additional observation: def-based plugs often just show the raw investment stat, except for SocketDetailsSelectedPlug which essentially has some logic from attachPlugStats duplicated.