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

Fix incorrect curse priority #3930

Merged
merged 8 commits into from
Jan 31, 2022

Conversation

Lothrik
Copy link
Contributor

@Lothrik Lothrik commented Jan 20, 2022

Fixes #2808.

Description of the problem being solved:

Curse on Hit from corruption implicits are always applied after Curse On Hit Support gems, and trigger mark when you hit mods on rings. Curses from rings appear to override curses from gloves. The earliest alphabetically-named curse has highest priority, up to the curse limit. Curses are applied in alphabetical order according to their internal name. Despair's internal name is Vulnerability and Vulnerability's internal name is NewVulnerability.

TL;DR:

  • If you have Curse on Hit gloves: the curse from your gloves should always override Hextouch + Elemental Weakness.
  • If you have a Frostbite on Hit ring, a Vulnerability on Hit ring, and Elemental Weakness on Hit gloves: Frostbite should override Vulnerability, and Vulnerability should override Elemental Weakness.
  • If you have Vulnerability + Elemental Weakness on Hit gloves, Elemental Weakness should override Vulnerability.

Almost everything I just listed above PoB gets wrong in 2.13.0, as it makes no attempt to prioritize curses alphabetically, prioritize curse on hit corruptions over anything, or prioritize ring curse on hit effects over glove curse on hit effects.

I'm not 100% confident about how I'm handling triggered Marks. The wiki claims they're prioritized below implicit Curse on Hit gloves, which makes sense.. sort of.. given that the trigger effect has a cooldown, but I'm suspicious about the exact mechanics involved.

Link to a build that showcases this PR:

https://pastebin.com/N4eu1SBZ

@Lothrik Lothrik added the bug Something isn't working label Jan 20, 2022
Copy link
Member

@Wires77 Wires77 left a comment

Choose a reason for hiding this comment

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

In hindsight, I should've made the changes as one suggestion, sorry if it's hard to read. In general, we use stringVar:match(pattern) instead of string.match(stringVar, pattern) for string functions and nil is falsy, so we can use if var then vs. if var ~= nil then

As a future PR, we might be able to construct the priority table from the export scripts, but this works for now. I'd like to see a reasonable fallback value if the curse isn't found, though

@Lothrik
Copy link
Contributor Author

Lothrik commented Jan 27, 2022

All of that should be fixed now, plus some other bugs I noticed while going over this again.

Copy link
Member

@Wires77 Wires77 left a comment

Choose a reason for hiding this comment

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

You had left/right swapped, but this looks good now

@Wires77 Wires77 merged commit f5eef71 into PathOfBuildingCommunity:dev Jan 31, 2022
@Lothrik Lothrik deleted the curse-priority-rework branch February 1, 2022 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Curse priority is incorrect for multiple corrupted curse implicits
2 participants