-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix rounding of scaled mods being incorrect in some cases #8862
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 rounding of scaled mods being incorrect in some cases #8862
Conversation
| end | ||
|
|
||
| function ModListClass:MergeMod(mod) | ||
| if mod.type == "BASE" or mod.type == "INC" then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt this was safe because the only places we merge mods are places that don't even have MORE modifiers, let alone multiple. If we end up being able to scale an item that has 2+ more multipliers of the same type though, we should revisit this to make sure it's accurate
| local scaledList = new("ModList") | ||
| scaledList:ScaleAddList(combinedList, scale) | ||
| for _, mod in ipairs(scaledList) do | ||
| combinedList:MergeMod(mod) | ||
| end | ||
| env.itemModDB:AddList(combinedList) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I duplicated this logic for the gloves even though it's not technically necessary. We should have this logic for all item scaling too for future proofing, most likely
PathOfBuildingCommunity/PathOfBuilding#8862 PathOfBuildingCommunity/PathOfBuilding#8772 PathOfBuildingCommunity/PathOfBuilding#8754 PathOfBuildingCommunity/PathOfBuilding#8739 Co-authored-by: LocalIdentity <localidentity2@gmail.com>
Mostlikely caused PathOfBuildingCommunity#8862
Most likely caused by PathOfBuildingCommunity#8862
PathOfBuildingCommunity/PathOfBuilding#8862 PathOfBuildingCommunity/PathOfBuilding#8772 PathOfBuildingCommunity/PathOfBuilding#8754 PathOfBuildingCommunity/PathOfBuilding#8739 Co-authored-by: LocalIdentity <localidentity2@gmail.com>
Fixes #8860 .
Description of the problem being solved:
The way scaling mods on items seems to work in game is that the mod is scaled, rounded towards 0, then added to the existing mod. i.e. mod + truncate(mod * scale) vs. truncate(mod * (1 + scale))
This is really only seen when the scalar is below 1, which only happens for Runegraft of the Bound (that I could tell)
Steps taken to verify a working solution:
Note that I only fixed this for Runegraft of the Bound, because this seems to only apply for scaling item effects. I also tested Mistress of Sacrifice with 25% Bone Offering and it gave 12% block in game. That doesn't match our formula above, so I didn't change the
ScaleAddModitself.Link to a build that showcases this PR:
Before screenshot:
After screenshot: