Conversation
There was a problem hiding this comment.
The CanUse part looks fine to me – but I think with the way ExecuteAction was designed, we might also need to address the "fallback" logic now.
At the moment, if Skill is called, the ExecuteAction checks for canUseSkill first, and if it fails, falls back to using Attack (which is desired).
Now, if Attack is called but fails at canUseAttack (e.g. a Jingliu in her Enhanced state), ideally we want to fall back to Skill, otherwise we won't have anything to return.
For that, I wouldn't do a big refactor for now, as this part might (or might not) see an overhaul someday.
| canUseAttack, err := mgr.engine.CanUseAttack(id) | ||
| if err != nil || !canUseAttack { | ||
| return target.ExecutableAction{}, err | ||
| } |
There was a problem hiding this comment.
might want to return a custom error message somewhere in case both canUseSkill and canUseAttack fail (which should never happen ingame)
2 participants
Fixes #269