Prevent APL prepull crashes, and add warnings instead

sim/core/api.go

@@ -16,7 +16,8 @@ func ComputeStats(csr *proto.ComputeStatsRequest) *proto.ComputeStatsResult {
 	if encounter == nil {
 		encounter = &proto.Encounter{}
 	}
-	_, raidStats, encounterStats := NewEnvironment(csr.Raid, encounter)
+
+	_, raidStats, encounterStats := NewEnvironment(csr.Raid, encounter, true)
 
 	return &proto.ComputeStatsResult{
 		RaidStats:      raidStats,

sim/core/apl.go

@@ -41,66 +41,80 @@ func (rot *APLRotation) ValidationWarning(message string, vals ...interface{}) {
 	rot.curWarnings = append(rot.curWarnings, warning)
 }
 
+// Invokes the fn function, and attributes all warnings generated during its invocation
+// to the provided warningsList.
+func (rot *APLRotation) doAndRecordWarnings(warningsList *[]string, isPrepull bool, fn func()) {
+	rot.parsingPrepull = isPrepull
+	fn()
+	if warningsList != nil {
+		*warningsList = append(*warningsList, rot.curWarnings...)
+	}
+	rot.curWarnings = nil
+	rot.parsingPrepull = false
+}
+
 func (unit *Unit) newAPLRotation(config *proto.APLRotation) *APLRotation {
 	if config == nil || config.Type != proto.APLRotation_TypeAPL {
 		return nil
 	}
 
 	rotation := &APLRotation{
-		unit: unit,
+		unit:                 unit,
+		prepullWarnings:      make([][]string, len(config.PrepullActions)),
+		priorityListWarnings: make([][]string, len(config.PriorityList)),
 	}
 
 	// Parse prepull actions
-	rotation.parsingPrepull = true
-	for _, prepullItem := range config.PrepullActions {
-		if !prepullItem.Hide {
-			doAtVal := rotation.newAPLValue(prepullItem.DoAtValue)
-			if doAtVal != nil {
-				doAt := doAtVal.GetDuration(nil)
-				if doAt > 0 {
-					rotation.ValidationWarning("Invalid time for 'Do At', ignoring this Prepull Action")
-				} else {
-					action := rotation.newAPLAction(prepullItem.Action)
-					if action != nil {
-						rotation.prepullActions = append(rotation.prepullActions, action)
-						unit.RegisterPrepullAction(doAt, func(sim *Simulation) {
-							action.Execute(sim)
-						})
+	for prepullIdx, prepullItem := range config.PrepullActions {
+		rotation.doAndRecordWarnings(&rotation.prepullWarnings[prepullIdx], true, func() {
+			if !prepullItem.Hide {
+				doAtVal := rotation.newAPLValue(prepullItem.DoAtValue)
+				if doAtVal != nil {
+					doAt := doAtVal.GetDuration(nil)
+					if doAt > 0 {
+						rotation.ValidationWarning("Invalid time for 'Do At', ignoring this Prepull Action")
+					} else {
+						action := rotation.newAPLAction(prepullItem.Action)
+						if action != nil {
+							rotation.prepullActions = append(rotation.prepullActions, action)
+							unit.RegisterPrepullAction(doAt, func(sim *Simulation) {
+								// Warnings for prepull cast failure are detected by running a fake prepull,
+								// so this action.Execute needs to record warnings.
+								rotation.doAndRecordWarnings(&rotation.prepullWarnings[prepullIdx], true, func() {
+									action.Execute(sim)
+								})
+							})
+						}
 					}
 				}
 			}
-		}
-
-		rotation.prepullWarnings = append(rotation.prepullWarnings, rotation.curWarnings)
-		rotation.curWarnings = nil
+		})
 	}
-	rotation.parsingPrepull = false
 
 	// Parse priority list
 	var configIdxs []int
 	for i, aplItem := range config.PriorityList {
-		if !aplItem.Hide {
-			action := rotation.newAPLAction(aplItem.Action)
-			if action != nil {
-				rotation.priorityList = append(rotation.priorityList, action)
-				configIdxs = append(configIdxs, i)
+		rotation.doAndRecordWarnings(&rotation.priorityListWarnings[i], false, func() {
+			if !aplItem.Hide {
+				action := rotation.newAPLAction(aplItem.Action)
+				if action != nil {
+					rotation.priorityList = append(rotation.priorityList, action)
+					configIdxs = append(configIdxs, i)
+				}
 			}
-		}
-
-		rotation.priorityListWarnings = append(rotation.priorityListWarnings, rotation.curWarnings)
-		rotation.curWarnings = nil
+		})
 	}
 
 	// Finalize
-	for _, action := range rotation.prepullActions {
-		action.Finalize(rotation)
-		rotation.curWarnings = nil
+	for i, action := range rotation.prepullActions {
+		rotation.doAndRecordWarnings(&rotation.prepullWarnings[i], true, func() {
+			action.Finalize(rotation)
+		})
 	}
 	for i, action := range rotation.priorityList {
-		action.Finalize(rotation)
-
-		rotation.priorityListWarnings[configIdxs[i]] = append(rotation.priorityListWarnings[configIdxs[i]], rotation.curWarnings...)
-		rotation.curWarnings = nil
+		rotation.doAndRecordWarnings(&rotation.priorityListWarnings[i], false, func() {
+			action.Finalize(rotation)
+		})
 	}
 
 	// Remove MCDs that are referenced by APL actions, so that the Autocast Other Cooldowns
@@ -113,23 +127,23 @@ func (unit *Unit) newAPLRotation(config *proto.APLRotation) *APLRotation {
 	}
 
 	// If user has a Prepull potion set but does not use it in their APL settings, we enable it here.
-	rotation.parsingPrepull = true
-	prepotSpell := rotation.GetAPLSpell(ActionID{OtherID: proto.OtherAction_OtherActionPotion}.ToProto())
-	rotation.parsingPrepull = false
-	if prepotSpell != nil {
-		found := false
-		for _, prepullAction := range rotation.allPrepullActions() {
-			if castSpellAction, ok := prepullAction.impl.(*APLActionCastSpell); ok &&
-				(castSpellAction.spell == prepotSpell || castSpellAction.spell.Flags.Matches(SpellFlagPotion)) {
-				found = true
+	rotation.doAndRecordWarnings(nil, true, func() {
+		prepotSpell := rotation.GetAPLSpell(ActionID{OtherID: proto.OtherAction_OtherActionPotion}.ToProto())
+		if prepotSpell != nil {
+			found := false
+			for _, prepullAction := range rotation.allPrepullActions() {
+				if castSpellAction, ok := prepullAction.impl.(*APLActionCastSpell); ok &&
+					(castSpellAction.spell == prepotSpell || castSpellAction.spell.Flags.Matches(SpellFlagPotion)) {
+					found = true
+				}
+			}
+			if !found {
+				unit.RegisterPrepullAction(-1*time.Second, func(sim *Simulation) {
+					prepotSpell.Cast(sim, nil)
+				})
 			}
 		}
-		if !found {
-			unit.RegisterPrepullAction(-1*time.Second, func(sim *Simulation) {
-				prepotSpell.Cast(sim, nil)
-			})
-		}
-	}
+	})
 
 	return rotation
 }

sim/core/cast.go

@@ -65,6 +65,15 @@ func (cast *Cast) EffectiveTime() time.Duration {
 type CastFunc func(*Simulation, *Unit)
 type CastSuccessFunc func(*Simulation, *Unit) bool
 
+func (spell *Spell) castFailureHelper(sim *Simulation, message string, vals ...interface{}) bool {
+	reason := fmt.Sprintf(spell.ActionID.String()+" failed to cast: "+message, vals...)
+	if sim.CurrentTime < 0 && spell.Unit.IsUsingAPL {
+		spell.Unit.Rotation.ValidationWarning(reason)
+		return false
+	}
+	panic(reason)
+}
+
 func (spell *Spell) makeCastFunc(config CastConfig) CastSuccessFunc {
 	return func(sim *Simulation, target *Unit) bool {
 		spell.CurCast = spell.DefaultCast
@@ -110,26 +119,26 @@ func (spell *Spell) makeCastFunc(config CastConfig) CastSuccessFunc {
 		if config.CD.Timer != nil {
 			// By panicking if spell is on CD, we force each sim to properly check for their own CDs.
 			if !spell.CD.IsReady(sim) {
-				panic(fmt.Sprintf("Trying to cast %s but is still on cooldown for %s, curTime = %s", spell.ActionID, spell.CD.TimeToReady(sim), sim.CurrentTime))
+				return spell.castFailureHelper(sim, "still on cooldown for %s, curTime = %s", spell.CD.TimeToReady(sim), sim.CurrentTime)
 			}
 			spell.CD.Set(sim.CurrentTime + spell.CurCast.CastTime + spell.CD.Duration)
 		}
 
 		if config.SharedCD.Timer != nil {
 			// By panicking if spell is on CD, we force each sim to properly check for their own CDs.
 			if !spell.SharedCD.IsReady(sim) {
-				panic(fmt.Sprintf("Trying to cast %s but is still on shared cooldown for %s, curTime = %s", spell.ActionID, spell.SharedCD.TimeToReady(sim), sim.CurrentTime))
+				return spell.castFailureHelper(sim, "still on shared cooldown for %s, curTime = %s", spell.SharedCD.TimeToReady(sim), sim.CurrentTime)
 			}
 			spell.SharedCD.Set(sim.CurrentTime + spell.CurCast.CastTime + spell.SharedCD.Duration)
 		}
 
 		// By panicking if spell is on CD, we force each sim to properly check for their own CDs.
 		if spell.CurCast.GCD != 0 && !spell.Unit.GCD.IsReady(sim) {
-			panic(fmt.Sprintf("Trying to cast %s but GCD on cooldown for %s, curTime = %s", spell.ActionID, spell.Unit.GCD.TimeToReady(sim), sim.CurrentTime))
+			return spell.castFailureHelper(sim, "GCD on cooldown for %s, curTime = %s", spell.Unit.GCD.TimeToReady(sim), sim.CurrentTime)
 		}
 
 		if hc := spell.Unit.Hardcast; hc.Expires > sim.CurrentTime {
-			panic(fmt.Sprintf("Trying to cast %s but casting/channeling %v for %s, curTime = %s", spell.ActionID, hc.ActionID, hc.Expires-sim.CurrentTime, sim.CurrentTime))
+			return spell.castFailureHelper(sim, "casting/channeling %v for %s, curTime = %s", hc.ActionID, hc.Expires-sim.CurrentTime, sim.CurrentTime)
 		}
 
 		if effectiveTime := spell.CurCast.EffectiveTime(); effectiveTime != 0 {
@@ -211,7 +220,7 @@ func (spell *Spell) makeCastFuncSimple() CastSuccessFunc {
 		if spell.CD.Timer != nil {
 			// By panicking if spell is on CD, we force each sim to properly check for their own CDs.
 			if !spell.CD.IsReady(sim) {
-				panic(fmt.Sprintf("Trying to cast %s but is still on cooldown for %s, curTime = %s", spell.ActionID, spell.CD.TimeToReady(sim), sim.CurrentTime))
+				return spell.castFailureHelper(sim, "still on cooldown for %s, curTime = %s", spell.CD.TimeToReady(sim), sim.CurrentTime)
 			}
 
 			spell.CD.Set(sim.CurrentTime + spell.CD.Duration)
@@ -220,7 +229,7 @@ func (spell *Spell) makeCastFuncSimple() CastSuccessFunc {
 		if spell.SharedCD.Timer != nil {
 			// By panicking if spell is on CD, we force each sim to properly check for their own CDs.
 			if !spell.SharedCD.IsReady(sim) {
-				panic(fmt.Sprintf("Trying to cast %s but is still on shared cooldown for %s, curTime = %s", spell.ActionID, spell.SharedCD.TimeToReady(sim), sim.CurrentTime))
+				return spell.castFailureHelper(sim, "still on shared cooldown for %s, curTime = %s", spell.SharedCD.TimeToReady(sim), sim.CurrentTime)
 			}
 
 			spell.SharedCD.Set(sim.CurrentTime + spell.SharedCD.Duration)

sim/core/environment.go

@@ -48,14 +48,14 @@ type Environment struct {
 	prepullActions []PrepullAction
 }
 
-func NewEnvironment(raidProto *proto.Raid, encounterProto *proto.Encounter) (*Environment, *proto.RaidStats, *proto.EncounterStats) {
+func NewEnvironment(raidProto *proto.Raid, encounterProto *proto.Encounter, runFakePrepull bool) (*Environment, *proto.RaidStats, *proto.EncounterStats) {
 	env := &Environment{
 		State: Created,
 	}
 
 	env.construct(raidProto, encounterProto)
 	raidStats := env.initialize(raidProto, encounterProto)
-	env.finalize(raidProto, encounterProto, raidStats)
+	env.finalize(raidProto, encounterProto, raidStats, runFakePrepull)
 
 	encounterStats := &proto.EncounterStats{}
 	for _, target := range env.Encounter.Targets {
@@ -142,7 +142,7 @@ func (env *Environment) initialize(raidProto *proto.Raid, encounterProto *proto.
 }
 
 // The finalization phase.
-func (env *Environment) finalize(raidProto *proto.Raid, _ *proto.Encounter, raidStats *proto.RaidStats) {
+func (env *Environment) finalize(raidProto *proto.Raid, _ *proto.Encounter, raidStats *proto.RaidStats, runFakePrepull bool) {
 	for _, finalizeEffect := range env.preFinalizeEffects {
 		finalizeEffect()
 	}
@@ -186,14 +186,26 @@ func (env *Environment) finalize(raidProto *proto.Raid, _ *proto.Encounter, raid
 
 	env.setupAttackTables()
 
+	env.State = Finalized
+
+	if runFakePrepull {
+		// Runs prepull only, for a single iteration. This lets us detect misconfigured
+		// prepull spells (e.g. GCD not available) in APL.
+		sim := newSimWithEnv(env, &proto.SimOptions{
+			Iterations: 1,
+		})
+		sim.Init()
+		sim.reset()
+		sim.PrePull()
+		sim.Cleanup()
+	}
+
 	for partyIdx, party := range env.Raid.Parties {
 		for _, player := range party.Players {
 			character := player.GetCharacter()
 			character.FillPlayerStats(raidStats.Parties[partyIdx].Players[character.PartyIndex])
 		}
 	}
-
-	env.State = Finalized
 }
 
 func (env *Environment) setupAttackTables() {

sim/core/sim.go

@@ -119,13 +119,16 @@ func runSim(rsr *proto.RaidSimRequest, progress chan *proto.ProgressMetrics, ski
 }
 
 func NewSim(rsr *proto.RaidSimRequest) *Simulation {
-	simOptions := rsr.SimOptions
+	env, _, _ := NewEnvironment(rsr.Raid, rsr.Encounter, false)
+	return newSimWithEnv(env, rsr.SimOptions)
+}
+
+func newSimWithEnv(env *Environment, simOptions *proto.SimOptions) *Simulation {
 	rseed := simOptions.RandomSeed
 	if rseed == 0 {
 		rseed = time.Now().UnixNano()
 	}
 
-	env, _, _ := NewEnvironment(rsr.Raid, rsr.Encounter)
 	return &Simulation{
 		Environment: env,
 		Options:     simOptions,

sim/rogue/rogue_test.go

@@ -131,7 +131,7 @@ func GenerateCriticalDamageMultiplierTestCase(
 		TalentsString: talents,
 	}, spec), nil, nil, nil)
 	encounter := core.MakeSingleTargetEncounter(0.0)
-	env, _, _ := core.NewEnvironment(raid, encounter)
+	env, _, _ := core.NewEnvironment(raid, encounter, false)
 	agent := env.Raid.Parties[0].Players[0]
 	rog := agent.(RogueAgent).GetRogue()
 	actualMultiplier := 0.0

sim/warlock/TestDemonology.results

@@ -6,7 +6,7 @@ character_stats_results: {
   final_stats: 1889.415
   final_stats: 1569.7
   final_stats: 1337.6
-  final_stats: 4285.664
+  final_stats: 4701.97114
   final_stats: 109
   final_stats: 379
   final_stats: 1773.98174