Obsidian Weapons gear bonus #70

[brickinthewall4138]

Closes #70, #92

[codecov]

Codecov Report

Patch coverage: 100% and no project coverage change.

Comparison is base (310537f) 99% compared to head (162ee3b) 99%.

Additional details and impacted files

@@           Coverage Diff           @@
##             master    #99   +/-   ##
=======================================
  Coverage        99%    99%           
- Complexity      480    501   +21     
=======================================
  Files            76     78    +2     
  Lines          1436   1473   +37     
  Branches        137    145    +8     
=======================================
+ Hits           1423   1460   +37     
  Partials         13     13           

Impacted Files	Coverage Δ
...alc/calc/gearbonus/BerserkerNecklaceGearBonus.java	100% <100%> (ø)
...osrs/dpscalc/calc/gearbonus/ObsidianGearBonus.java	100% <100%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[LlemonDuck]

Don't catch DpsComputeExceptions during the compute flow.

[brickinthewall4138]

Fair enough, I added this in anticipation of null spells as in the other PR so it can be removed without complaint.

[LlemonDuck]

These are multiplicative with integer truncation in the middle, although the integer truncation behaviour needs to be changed in GearBonusComputable.

As long as you change the two bonuses to be in separate classes, this will automatically be done.

[brickinthewall4138]

As long as you change the two bonuses to be in separate classes, this will automatically be done.

Will do,

although the integer truncation behaviour needs to be changed in GearBonusComputable.

A note on that front. I'm looking at #82 and #78 on one of my local branches.

As you seem to be aware, currently gear bonuses are aggregated naively without any considerations for integer truncation. This is part of the reason for the plugin's inaccuracy as seen in these issues. AggregateGearBonusesComputable will have to go, or otherwise mutate into some Set or whatever.

There's also another problem, wherein the salve amulet and elite void mage set aren't actually applied as a gear bonus for magic damage, it's actually like an additive 20%/2.5% increase (respectively) to the magic damage bonus.

These two otherwise benefit from remaining under the GearBonus structure. However keeping them under there means GearBonuses can no longer remain anonymous as far as whatever new aggregator will be concerned, as these two specific bonuses require special treatment. I was thinking of adding a basic ID field with each GearBonus assigned an integer in a separate file (GearBonusID), but this is pretty horrible and I'm probably missing the forest for the trees, it's an inelegant solution but it would work for resolving the second problem. The slayer helm and tomes still behave like normal GearBonuses for the purposes of magic - it's just the salve amulet / void set that misbehave.

Sorry for the mountains of text, your thoughts?

PS: I'm fine to keep our conversations here, but I'm open to moving elsewhere if it's more convenient for you.

[LlemonDuck]

There's also another problem, wherein the salve amulet and elite void mage set aren't actually applied as a gear bonus for magic damage, it's actually like an additive 20%/2.5% increase (respectively) to the magic damage bonus.

Do you have a source on this? Are all magic damage effects applied as such? (e.g. smoke battlestaff? which seems to be one of the least documented damage bonuses)

These two otherwise benefit from remaining under the GearBonus structure [...snip]

If they're uniquely applied relative to other GearBonuses, it'll be good to set up a StatsBonus system that modifies the player stats before going into bonuses. The Tumeken's Shadow effect is implemented poorly but would be able to be rolled in properly to that.

PS: I'm fine to keep our conversations here, but I'm open to moving elsewhere if it's more convenient for you.

Discord is easier (LlemonDuck#3306), but I'll probably forget to reply sometimes there so for discussion like this we can use Discord, and for code that's actually been written let's keep it on GH.

[brickinthewall4138]

Do you have a source on this? Are all magic damage effects applied as such? (e.g. smoke battlestaff? which seems to be one of the least documented damage bonuses)

My source is the holy grail that everyone seems to agree is correct, it's used by the existing spreadsheets afaik:
https://web.archive.org/web/20190905124128/http://webcache.googleusercontent.com/search?q=cache:http://services.runescape.com/m=forum/forums.ws?317,318,712,65587452

I found it by checking the references on this wiki page:
https://oldschool.runescape.wiki/w/Maximum_magic_hit

Personally in terms of in-game verification, I can't check the smoke battlestaff (ironman btw ;), but I'd be inclined to believe this forum post which states it's a magic damage bonus. The salve amulet however I've seen for myself is certainly a magic damage bonus. Void mage is even more transparent as it shows as a magic damage bonus in the equipment UI in-game. I'm pretty sure as I say that slayer helms and tomes still work with truncation after their effects.

If you have a bonded account with a smoke battlestaff, you could confirm by casting fire surge with a tormented bracelet, imbued god cape, and smoke battlestaff of course, on a POH dummy. If it's a gear bonus, it will be int(int(24 * 1.07) * 1.1) = 27. If it's added to magic bonus, it will be int(24*1.17) = 28.

If they're uniquely applied relative to other GearBonuses, it'll be good to set up a StatsBonus system that modifies the player stats before going into bonuses. The Tumeken's Shadow effect is implemented poorly but would be able to be rolled in properly to that.

That does sound like it could be more elegant, I'll look into that when I have time.

[brickinthewall4138]

I felt I ought to move the checks for the besrker necklace / obsidian armour into isApplicable rather than compute, hence the second commit, not sure if you thought it was better the way it was or didn't notice? This way makes more sense to me, since they're all-or-nothing bonuses unlike e.g. crystal armour / inquisitor armour. I'm going to try rectify whatever problem codecov's noticed without breaking anything else now.