Skip to content

SPELL - Add JumpTargets field to spell for targetcapped abilties.#1384

Draft
ncberman wants to merge 3 commits intowowsims:masterfrom
ncberman:spell_jumptargets
Draft

SPELL - Add JumpTargets field to spell for targetcapped abilties.#1384
ncberman wants to merge 3 commits intowowsims:masterfrom
ncberman:spell_jumptargets

Conversation

@ncberman
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread sim/core/spell.go
Comment on lines +644 to +658
targetsToHit := min(spell.JumpTargets, sim.Environment.GetNumTargets())
for targetIndex := int32(0); targetIndex < targetsToHit; targetIndex++ {
// Not sure if we want to split this flag into its own?
// Both are used to optimize away unneccesery calls and 99%
// of the time are gonna be used together. For now just in one
if !spell.Flags.Matches(SpellFlagNoOnCastComplete) {
spell.Unit.OnApplyEffects(sim, target, spell)
}

spell.ApplyEffects(sim, target, spell)

spell.ApplyEffects(sim, target, spell)
if targetIndex < targetsToHit-1 {
target = sim.Environment.NextTargetUnit(target)
}
}
Copy link
Copy Markdown
Contributor

@FelixPflaum FelixPflaum Mar 24, 2025

Choose a reason for hiding this comment

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

The only spells that chain in the sim are, I think, Chain Lightning, Multi Shot and Cleave. The better place to have that functionality is in the ApplyEffects function of the individual spells as it is done currently. Just replace the hardcoded chain count there instead.

Not to mention CL also requires the target number to calculate its dmg. That would even require adding an additional parameter for just those few spells.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with the above. This functionality is very invasive to be in-here for the sake of a couple of spells which can just implement it into its own effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants