search

xyzz / acquisition

http://get.acquisition.today/
GNU General Public License v3.0
270 stars 100 forks source link

Elevate chaos damage support #495

Closed DaneelTrevize closed 5 years ago

DaneelTrevize commented 5 years ago

Add Chaos Damage and Chaos DPS columns. Modify total DPS value to include Chaos DPS value. Add Mods filter support for % Chaos Damage, Added Chaos Damage (to Attacks, to Spells). Removed fire-combo-specific filters, for lack of justification of special-casing them vs other damage types. Add Chaos DPS fields/filter to the Offense section of the search UI.

ericsium commented 5 years ago

Removing the fire combo specific filters seems unrelated to the issue, can you remove from this pull?

That could be cleaned up as part of a separate enhancement request - I'm not sure why we'd want to remove something that was already there and that some users may be used to using. If there are some inconsistencies I'd figure we'd want to instead add more filters for other damage types rather than removing these existing ones?

DaneelTrevize commented 5 years ago

I saw it as the other way, that supporting fire+area, and burning, pseudo/combo mods was inconsistently prioritising fire-based ailments. As this overall PR was about making Chaos damage as consistently supported as other damage types, it seemed reasonable to remove the bias towards fire in this dropdown while we're also adding several new entries. Cold, Lightning and Chaos certainly have several more meta-current interactions with Area and/or DoT modifiers, the burning bias seemed outdated. I'm not saying I can't/won't try clean up this part of the PR, just playing devil's advocate for this first go around.

P.S. perhaps a generic % increased Area Damage filter would be possible/best? P.P.S. Unless I'm mistaken, the live Fire Area Damage filter is bugged anyway, as it doesn't let through e.g. Carcass Jack. So it'd need redoing anyway.

DaneelTrevize commented 5 years ago

OK apparently this branch isn't going to squash nicely around that master merge, and as soon as other PRs are accepted that also add columns to the search result, git will have a merge conflict over not magically knowing how/which order to have all the new columns together, because no single file defined that until it defines both the chaos column and the e.g. Shaper/Elder column, but you guys didn't want these PRs as a series of chained branches that would do so, so I guess I'll have to wait for each to be accepted and then rebase/recreate the dependant/conflicting branches afterwards so it's clear..?

ericsium commented 5 years ago

Maybe chained branches would actually have been better and easier, I had assumed there would have been merge conflicts on master with this approach but thinking about it more there probably wouldn't have been as all the branches should have included the same shared changes. We could do a test to see how that works out at some point. Maybe xyzz can comment if this is what we should do going forward. Could at least try a test of this at some point.

Yes, once a few make it onto master you may need to rebase your pull branches for clean merges.

Sorry if I guided you wrong, I think many will appreciate the work you're putting in.

ericsium commented 5 years ago

I'm still not quite following on why you want to remove the fire mods. These are just attempting to emulate what's currently available on the poe.trade filters ([total] mods):

[total] Adds # fire damage to attacks [total] %# increased fire area damage [total] %# increased burning damage

It's true that fire area damage is missing area damage which could just be added to fix the issue:

{ "#% increased Fire Area Damage", "#% increased Fire Damage", "#% increased Elemental Damage" },

I'm not sure what you mean by how fire based mods are being 'prioritized'? Do you mean that they exist for fire and not other damage types and are inconsistent even on poe.trade?

I'll let xyzz decide if this is fine and merge the pull to master. Everything else looks okay here.

DaneelTrevize commented 5 years ago

I relent, we have the stupid Leo low-level mod line, why not have Burning. I also added generic % damage mod lines for the other types, so users aren't restricted to spells/attacks, as well as a dedicated DoT, and including % Global Physical in % Physical. Makes finding a bunch of jewels and uniques a lot easier.

I'm getting the impression we'll want a way to signify to users which mod lines are pseudo/composite ones, and which are exact text matching only.