Test/itemeffects #1491

ToxicKevinFerm · 2025-05-08T22:01:25Z

No description provided.

NerdEgghead

Great start, this will hugely simplify our very messy on-use item factories once it's finished!

NerdEgghead · 2025-05-09T06:34:08Z

proto/db.proto

 	map<int32, ScalingItemProperties> scaling_options = 13; // keys are the all ItemLevelState variants that this item could potentially have
+	repeated ItemEffect item_effects = 14;


Does this need to be an array? Are there any relevant cases where we need to process multiple effects for a single item?

@NerdEgghead I think trinkets like this:

https://www.wowhead.com/mop-classic/item=105472/assurance-of-consequence

https://www.wowhead.com/mop-classic/item=105527/haromms-talisman

https://www.wowhead.com/mop-classic/item=105459/fusion-fire-core

and more

@NerdEgghead I think trinkets like this:

https://www.wowhead.com/mop-classic/item=105472/assurance-of-consequence

https://www.wowhead.com/mop-classic/item=105527/haromms-talisman

https://www.wowhead.com/mop-classic/item=105459/fusion-fire-core

and more

Trinkets like these will require custom implementations for the second effect regardless, it's only the simple stat effect that we can handle automatically with factory functions. If those are the only examples, then it would be better to just remove the complex effect during pre-processing and dump only the stat buff effect.

NerdEgghead · 2025-05-09T06:36:51Z

proto/spell.proto

+enum ItemEffectType {
+	NONE   = 0;
+	PROC   = 1;
+	ON_USE = 2;
+  }


Stick with the same naming convention we've been using for our other enums:

enum ItemEffectType { EffectTypeUnknown = 0; EffectTypeProc = 1; EffectTypeOnUse = 2; }

NerdEgghead · 2025-05-09T06:38:16Z

proto/spell.proto

+  // Contains only the Enchant info needed by the sim.
+message SimEnchant {
+	int32 effect_id = 1;
+	repeated double stats = 2;
+	ItemEffect enchant_effect = 3;
+}


It would make more sense for this to live in db.proto, unless that would cause import cycles.

NerdEgghead · 2025-05-09T06:41:19Z

proto/spell.proto

+	int32            spell_id         = 1;
+	string label = 9;


Call these buff_id and buff_name for clarity.

NerdEgghead · 2025-05-09T06:42:31Z

proto/spell.proto

+	int32 rppm_scale = 4;
+	map<int32, double> spec_modifiers = 5;


Clarify what the keys represent in a comment.

NerdEgghead · 2025-05-09T11:17:54Z

tools/database/dbc/item_effect.go

+	src := total.ToProtoMap()
+
+	m := make(map[int32]float64, len(src))
+	maps.Copy(m, src)
+
+	return &proto.ScalingItemEffectProperties{Stats: m}


Can you just directly populate the Stats field with src rather than allocating another map and copying to it first?

NerdEgghead · 2025-05-09T11:27:07Z

tools/database/dbc/item_effect.go

+func MergeItemEffectsForAllStates(parsed *proto.UIItem) []*proto.ItemEffect {
+	itemID := int(parsed.Id)
+	raws := dbcInstance.ItemEffectsByParentID[itemID]
+	var merged []*proto.ItemEffect
+
+	for idx := range raws {
+		var base *proto.ItemEffect
+
+		for key, props := range parsed.ScalingOptions {
+			state := proto.ItemLevelState(key)
+			ilvl := int(props.Ilvl)
+			slice := ParseItemEffects(itemID, ilvl, state)
+			eff := slice[idx]
+
+			if base == nil {
+				base = eff
+			} else {
+				base.ScalingOptions[key] = eff.ScalingOptions[key]
+			}
+		}
+		if base == nil {
+			continue
+		}
+
+		if len(base.ScalingOptions) > 0 {
+			merged = append(merged, base)
+		}
+	}
+	return merged
+}


Ok I see where you're handling the full set of ItemLevelState options now. This seems very inefficient though, since you're allocating and populating a whole other proto.ItemEffect struct for every variant of the item. Wouldn't it be better to create the base proto.ItemEffect only once per item, and then only update the ScalingOptions map within the loop over ItemLevelState options rather than merging a full proto?

Yeah I built myself into a bit of a corner when I realized I had to recursively step through spell triggers and also scale the stats array to the item level 😄

NerdEgghead · 2025-05-09T11:38:26Z

tools/database/dbc/spell_effect.go

+	case effect.EffectAura == A_MOD_RANGED_ATTACK_POWER:
+		if scalesWithIlvl && effect.Coefficient != 0 && spell.Attributes[8]&0x1000 != 0 {


Pre-cache this check before the switch rather than copy-pasting it three times:

useRandPropPoints := scalesWithIlvl && (effect.Coefficient != 0) && (spell.Attributes[8]&0x1000 != 0)

NerdEgghead · 2025-05-09T11:43:34Z

tools/database/dbc/spell_effect.go

+			propPoints := dbcInstance.RandomPropertiesByIlvl[ilvl][proto.ItemQuality_ItemQualityEpic][0] // Epic 0 for items
+			stats[proto.Stat_StatRangedAttackPower] = math.Floor(float64(propPoints) * effect.Coefficient)


Likewise, define a helper for this calculation so you're only coding the logic once:

func (effect *SpellEffect) CalcRandomPropStatValue(ilvl int) float64 { propPoints := dbcInstance.RandomPropertiesByIlvl[ilvl][proto.ItemQuality_ItemQualityEpic][0] // Epic 0 for items return math.Floor(float64(propPoints) * effect.Coefficient) }

NerdEgghead · 2025-05-09T11:46:56Z

tools/database/dbc/spell_effect.go

 			if statMod := RatingModToStat[rating]; statMod != -1 {
+				if scalesWithIlvl && effect.Coefficient != 0 && spell.Attributes[8]&0x1000 == 0 {


You're doing spell.Attributes[8]&0x1000 == 0 here when it's been != 0 the previous three times, is that intended?

On branch test/itemeffects Changes to be committed: modified: ui/core/components/individual_sim_ui/bulk_tab.tsx modified: ui/core/proto_utils/gear.ts

On branch test/itemeffects Changes to be committed: modified: proto/common.proto modified: sim/core/TestProtoVersioning.results modified: sim/death_knight/blood/TestBlood.results modified: sim/death_knight/frost/TestFrost.results modified: sim/death_knight/unholy/TestUnholy.results modified: sim/druid/balance/TestBalance.results modified: sim/druid/feral/TestFeral.results modified: sim/druid/guardian/TestGuardian.results modified: sim/hunter/beast_mastery/TestBM.results modified: sim/hunter/marksmanship/TestMM.results modified: sim/hunter/survival/TestSV.results modified: sim/mage/arcane/TestArcane.results modified: sim/mage/fire/TestFire.results modified: sim/paladin/protection/TestProtection.results modified: sim/paladin/retribution/TestRetribution.results modified: sim/priest/shadow/TestShadow.results modified: sim/rogue/assassination/TestAssassination.results modified: sim/rogue/combat/TestCombat.results modified: sim/rogue/subtlety/TestSubtlety.results modified: sim/shaman/elemental/TestElemental.results modified: sim/shaman/enhancement/TestEnhancement.results modified: sim/warlock/affliction/TestAffliction.results modified: sim/warlock/demonology/TestDemonology.results modified: sim/warlock/destruction/TestDestruction.results modified: sim/warrior/arms/TestArms.results modified: sim/warrior/fury/TestFury.results modified: sim/warrior/protection/TestProtectionWarrior.results

StatBuffAura label. Fixes test failures in configs with a Normal + Heroic version of the same trinket both equipped. On branch test/itemeffects Changes to be committed: modified: sim/common/shared/shared_utils.go modified: sim/death_knight/blood/TestBlood.results modified: sim/mage/arcane/TestArcane.results modified: sim/paladin/retribution/TestRetribution.results modified: sim/rogue/assassination/TestAssassination.results modified: sim/rogue/combat/TestCombat.results modified: sim/rogue/subtlety/TestSubtlety.results modified: sim/warrior/arms/TestArms.results

…to test/itemeffects

On branch test/itemeffects Changes to be committed: modified: sim/common/cata/stat_bonus_cds.go modified: sim/common/shared/shared_utils.go

On branch test/itemeffects Changes to be committed: modified: sim/common/cata/enchant_effects.go modified: sim/common/cata/stat_bonus_procs.go modified: sim/common/shared/shared_utils.go modified: sim/common/tbc/stat_bonus_procs.go modified: sim/common/wotlk/enchant_effects.go modified: sim/common/wotlk/stat_bonus_procs.go

NerdEgghead

Nearly there!

NerdEgghead · 2025-05-27T01:46:31Z

hjej.xt

What is this file? Does it need to be committed?

NerdEgghead · 2025-05-27T01:47:41Z

proto/spell.proto

+	string buff_name = 9;
+	ItemEffectType   type            = 2;


I don't think the type field is actually used anywhere, can it be deleted?

NerdEgghead · 2025-05-27T01:49:20Z

proto/spell.proto

+	// The key represents a Class enum key
+	map<int32, double> spec_modifiers = 5;


Should this be called class_modifiers instead if the keys are classes rather than specs?

It should actually be a spec key so i fix it

NerdEgghead · 2025-05-27T01:53:58Z

sim/common/shared/shared_utils.go

-	factory_StatBonusEffect(config, func(agent core.Agent) ExtraSpellInfo {
+	factory_StatBonusEffect(config, func(agent core.Agent, _ proto.ItemLevelState) ExtraSpellInfo {


Just a note that if these types of trinkets are still relevant in MoP, then we should probably convert the MinDmg, MaxDmg, and BonusCoefficient fields of shared.DamageEffect from floats into map[proto.ItemLevelState]float64 types.

NerdEgghead · 2025-05-27T02:01:19Z

sim/common/shared/shared_utils.go

-			procSpell = extraSpell(agent)
+			procSpell = extraSpell(agent, proto.ItemLevelState_Base)


You should call this with the actual itemLevelState value for generality, so that the code will still work in MoP if we generalize shared.DamageEffect to include scaling properties.

NerdEgghead · 2025-05-27T03:08:03Z

tools/database/dbc/item_effect.go

+// Parses a UIItem and loops through Scaling Options for that item.
+func MergeItemEffectsForAllStatesNew(parsed *proto.UIItem) *proto.ItemEffect {


Don't think you need the "New" in the name anymore.

NerdEgghead · 2025-05-27T03:10:25Z

tools/database/dbc/item_effect.go

+		if parsed.Id == 92127 {
+			fmt.Println("hello2", parsed, parsed.Ilvl, opt.Ilvl, opt)
+		}


Maybe a holdover from debugging?

NerdEgghead · 2025-05-27T03:14:14Z

tools/database/dbc/item_effect.go

+
+func realPpmScale(spell Spell) int {


You probably want to take in a *Spell pointer here instead, in order to avoid copying the input on every call.

NerdEgghead · 2025-05-27T03:15:44Z

tools/database/dbc/item_effect.go

+
+func realPpmModifier(spell Spell, itemLevel int) (float64, map[int32]float64) {


NerdEgghead · 2025-05-27T03:21:07Z

tools/database/dbc/spell_effect.go

-const BASE_LEVEL = 85
+const BASE_LEVEL = 90


Is this maybe why you were getting the wrong stat values for the boosted trinkets?

No, trinkets dont use base level

NerdEgghead · 2025-05-27T18:15:04Z

proto/spell.proto

+	int32            effect_duration = 2;  // seconds
+


Call this one effect_duration_ms as well so that the units are consistent across the message.

NerdEgghead · 2025-05-27T18:17:04Z

sim/core/character.go

 func (character *Character) GetConjuredCD() *Timer {
-	return character.GetOrInitTimer(&character.conjuredCD)
+	return character.GetOrInitSpellCategoryTimer(30)
 }
 func (character *Character) GetPotionCD() *Timer {
 	return character.GetOrInitTimer(&character.potionCD)


Make the same change for GetPotionCD() as well where you use the correct category ID for it, so that you can also delete potionCD from the Character struct.

sim/druid/guardian/TestGuardian.results

tools/database/dbc/enums.go

NerdEgghead · 2025-05-27T18:39:57Z

tools/database/dbc/item_effect.go

+		for _, se := range dbcInstance.SpellEffects[id] {
+			if s := se.ParseStatEffect(sp.HasAttributeAt(11, 0x4), itemLevel); s != &emptyStats {


I don't think s != &emptyStats can ever evaluate false, since you're comparing pointers rather than the arrays themselves. So even if s points to another empty Stats array, the expression will still evaluate as true.

NerdEgghead · 2025-05-27T18:54:09Z

proto/spell.proto

+	double 		ppm = 3;
+	int32 rppm_scale = 4;


Is rppm_scale here intended to represent a core.RppmModifier? If so, then it would be cleaner to make RppmScalingType a proto enum rather than a back-end type.

NerdEgghead · 2025-05-27T19:03:53Z

tools/database/dbc/spell_effect.go

+	spell := dbcInstance.Spells[effect.SpellID]
+	// if not we get class scaling based on the spell
 	scale := effect.ScalingClass()
-	spell := dbcInstance.Spells[effect.ID]
+	return dbcInstance.SpellScalings[core.TernaryInt(spell.MaxScalingLevel > BASE_LEVEL, BASE_LEVEL, spell.MaxScalingLevel)].Values[scale]


core.TernaryInt(spell.MaxScalingLevel > BASE_LEVEL, BASE_LEVEL, spell.MaxScalingLevel) can be simplified to just min(spell.MaxScalingLevel, BASE_LEVEL).

Test/itemeffects

neteyesking added 8 commits May 7, 2025 22:05

Add itemeffects

d61878c

Hi

a59e4d7

Fixes

0c5033d

Add rppm and maxstacks

1a7da20

Run item effects and enchants from DB

1189160

Correct scaling selector

bbe6768

Clean up

026e1b3

Fix imports

6f4b4b5

NerdEgghead reviewed May 9, 2025

View reviewed changes

ToxicKevinFerm added 3 commits May 13, 2025 23:29

Fixes

858a8c1

Trying something

d7b778f

Cleanup and fix tests

98f4799

github-actions bot added Hunter Rogue Shaman labels May 17, 2025

NerdEgghead added 2 commits May 17, 2025 10:22

Fix build errors

ea33d9a

On branch test/itemeffects Changes to be committed: modified: ui/core/components/individual_sim_ui/bulk_tab.tsx modified: ui/core/proto_utils/gear.ts

github-actions bot added Mage Paladin Priest Druid Warlock Warrior labels May 17, 2025

Merge branch 'master' into test/itemeffects

765f258

github-actions bot removed Priest Warlock labels May 17, 2025

NerdEgghead added 2 commits May 17, 2025 18:53

Merge branch 'master' into test/itemeffects

6da8bea

github-actions bot removed Mage Paladin labels May 18, 2025

Fix issues

883e24a

github-actions bot added Mage Paladin Priest Druid Warlock Warrior labels May 25, 2025

NerdEgghead mentioned this pull request May 25, 2025

Miscellaneous trinket fixes #1502

Merged

NerdEgghead and others added 2 commits May 25, 2025 12:18

Merge branch 'master' into test/itemeffects

378976f

Fix test passing and remove coefficient scaling

fca85a2

github-actions bot removed the Priest label May 25, 2025

NerdEgghead added 2 commits May 25, 2025 16:40

Merge branch 'master' into test/itemeffects

d1ee7ac

Merge branch 'test/itemeffects' of https://github.com/wowsims/cata in…

fea5316

…to test/itemeffects

github-actions bot removed Mage Paladin Warlock Warrior labels May 25, 2025

NerdEgghead added 2 commits May 25, 2025 21:09

Delete deprecated on-use factory functions

6d957b7

On branch test/itemeffects Changes to be committed: modified: sim/common/cata/stat_bonus_cds.go modified: sim/common/shared/shared_utils.go

NerdEgghead requested changes May 27, 2025

View reviewed changes

Addressed PR comments

8661a52

github-actions bot added the Warrior label May 27, 2025

Fixes

a7e910d

NerdEgghead requested changes May 27, 2025

View reviewed changes

ToxicKevinFerm added 2 commits May 27, 2025 22:12

Fix comments

b18d574

Fix RPPM

89ac11b

NerdEgghead approved these changes May 28, 2025

View reviewed changes

NerdEgghead merged commit 60386b3 into master May 28, 2025
2 checks passed

NerdEgghead deleted the test/itemeffects branch May 28, 2025 01:35

ToxicKevinFerm pushed a commit to wowsims/mop that referenced this pull request May 28, 2025

Merge pull request wowsims#1491 from wowsims/test/itemeffects

1298967

Test/itemeffects

		map<int32, ScalingItemProperties> scaling_options = 13; // keys are the all ItemLevelState variants that this item could potentially have
		repeated ItemEffect item_effects = 14;

		case effect.EffectAura == A_MOD_RANGED_ATTACK_POWER:
		if scalesWithIlvl && effect.Coefficient != 0 && spell.Attributes[8]&0x1000 != 0 {

		propPoints := dbcInstance.RandomPropertiesByIlvl[ilvl][proto.ItemQuality_ItemQualityEpic][0] // Epic 0 for items
		stats[proto.Stat_StatRangedAttackPower] = math.Floor(float64(propPoints) * effect.Coefficient)

		if statMod := RatingModToStat[rating]; statMod != -1 {
		if scalesWithIlvl && effect.Coefficient != 0 && spell.Attributes[8]&0x1000 == 0 {

		// The key represents a Class enum key
		map<int32, double> spec_modifiers = 5;

		factory_StatBonusEffect(config, func(agent core.Agent) ExtraSpellInfo {
		factory_StatBonusEffect(config, func(agent core.Agent, _ proto.ItemLevelState) ExtraSpellInfo {

		procSpell = extraSpell(agent)
		procSpell = extraSpell(agent, proto.ItemLevelState_Base)

		// Parses a UIItem and loops through Scaling Options for that item.
		func MergeItemEffectsForAllStatesNew(parsed proto.UIItem) proto.ItemEffect {


		func realPpmModifier(spell Spell, itemLevel int) (float64, map[int32]float64) {

		for _, se := range dbcInstance.SpellEffects[id] {
		if s := se.ParseStatEffect(sp.HasAttributeAt(11, 0x4), itemLevel); s != &emptyStats {

		const BASE_LEVEL = 85
		const BASE_LEVEL = 90

Test/itemeffects #1491

Test/itemeffects #1491

Uh oh!

Conversation

ToxicKevinFerm commented May 8, 2025

Uh oh!

NerdEgghead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NerdEgghead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone