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 support for Inevitable Critical Support #603

Merged
merged 7 commits into from
Jan 27, 2025

Conversation

jjbi123
Copy link
Contributor

@jjbi123 jjbi123 commented Jan 25, 2025

Fixes #298

Description of the problem being solved:

  • Inevitable Critical support gem had no support

Link to a build that showcases this PR: https://maxroll.gg/poe2/pob/vjqam0y9

Before screenshot:

image

After screenshot:

image

image

@Blitz54
Copy link
Contributor

Blitz54 commented Jan 25, 2025

EDIT: Cleaner way to do it, still might be a better way but I think this is good.

modList:NewMod("Multiplier:SecondsSinceInevitableCrit", "BASE", m_min(val, 4), "Config")

Copy link
Contributor

@PJacek PJacek left a comment

Choose a reason for hiding this comment

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

You should use limitTotal. It makes the limit apply to the total value after multiplication.
I'm not convicted by this implementation. Do we really expect users to figure out if they crit 2 or 3 seconds ago?
In my opinion a checkbox would be a better solution. At least until someone good enough at maths figures out how to calculate the effective impact.

@@ -2060,6 +2060,14 @@ skills["SupportInevitableCriticalsPlayer"] = {
label = "Inevitable Critical",
incrementalEffectiveness = 0.092720001935959,
statDescriptionScope = "gem_stat_descriptions",
statMap = {
["support_inevitable_criticals_critical_strike_chance_+%_per_second"] = {
mod("CritChance", "INC", nil, 0, 0, { type = "Multiplier", var = "SecondsSinceInevitableCrit", limitVar = "InevitableCritCap" }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mod("CritChance", "INC", nil, 0, 0, { type = "Multiplier", var = "SecondsSinceInevitableCrit", limitVar = "InevitableCritCap" }),
mod("CritChance", "INC", nil, 0, 0, { type = "Multiplier", var = "SecondsSinceInevitableCrit", limitVar = "InevitableCritCap", limitTotal = true }),

@@ -543,6 +543,14 @@ statMap = {
#skill SupportInevitableCriticalsPlayer
#startSets
#set SupportInevitableCriticalsPlayer
statMap = {
["support_inevitable_criticals_critical_strike_chance_+%_per_second"] = {
mod("CritChance", "INC", nil, 0, 0, { type = "Multiplier", var = "SecondsSinceInevitableCrit", limitVar = "InevitableCritCap" }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mod("CritChance", "INC", nil, 0, 0, { type = "Multiplier", var = "SecondsSinceInevitableCrit", limitVar = "InevitableCritCap" }),
mod("CritChance", "INC", nil, 0, 0, { type = "Multiplier", var = "SecondsSinceInevitableCrit", limitVar = "InevitableCritCap", limitTotal = true }),

@jjbi123 jjbi123 marked this pull request as ready for review January 25, 2025 17:13
@jjbi123
Copy link
Contributor Author

jjbi123 commented Jan 25, 2025

You should use limitTotal. It makes the limit apply to the total value after multiplication. I'm not convicted by this implementation. Do we really expect users to figure out if they crit 2 or 3 seconds ago? In my opinion a checkbox would be a better solution. At least until someone good enough at maths figures out how to calculate the effective impact.

@PJacek thanks! limitTotal makes sense. Regarding the checkbox vs number I can only speak for myself as a monk but I would prefer the ability to see just how much my crit chance is affected by the second. For example, I have it on Falling Thunder, and when I throw down a fully "charged" attack at 4 seconds I know it'll always crit, but I also get power charges back from that hit, so I could immediately Falling Thunder again, and would be nice to know what my crit chance is, say 1-2 seconds after my last hit, or if I should just wait a bit to get it fully charged again. Idk, given that PoB is all about customization and nitty-gritty stuff this seems to be in line with the general philosophy to err on the side of giving users more control rather than abstracting functionality. Thoughts?

@jjbi123 jjbi123 requested a review from PJacek January 26, 2025 23:17
@LocalIdentity LocalIdentity added the enhancement New feature, calculation, or mod label Jan 27, 2025
Copy link
Contributor

@LocalIdentity LocalIdentity left a comment

Choose a reason for hiding this comment

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

I made some changes to the location of the config as we keep the skills section of the config page in alphabetical order.
Also made the config grant the crit recently condition if the value is less than 4s

@LocalIdentity LocalIdentity merged commit 74529a4 into PathOfBuildingCommunity:dev Jan 27, 2025
2 checks passed
@LocalIdentity LocalIdentity changed the title Add support for Inevitable Critical support gem Add support for Inevitable Critical Support Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, calculation, or mod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Inevitable Critical Support
4 participants