Aldriana / ShadowCraft-Engine

Calculations backend for ShadowCraft, a WoW theorycraft project.
GNU Lesser General Public License v3.0
37 stars 22 forks source link

Procs with multiple proc behaviors #75

Open Aldriana opened 13 years ago

Aldriana commented 13 years ago

We need some way of dealing with procs that have more than one proc rate/behavior. The simple example of this is Avalanche, which procs off weapon-based attacks at 5 PPM but spell-based attacks at 15%. The harder example is Hurricane, which is similar but the spell proc has an ICD while the melee proc does not. If someone wants to think about an elegant way of handling this, that'd be great.

dazer commented 13 years ago

Would it make sense to instantiate two proc objects? 'avalanche_spell' and 'avalanche_melee' each with its own proc behaviour. Say the caller wants avalanche, so the constructor finds 'avalanche': {'stat': 'two_procs_flag'}and then builds two independent procs from 'avalanche_spell': {...}, avalanche_melee: {...} I think this would work as long as one version of the proc doesn't trigger the icd of the other (so far Avalanche has none)

Aldriana commented 13 years ago

That'd be one approach, yes. Another would be to define a ProcBehavior class holding the trigger, proc rate/ppm, and icd and then have each proc contain one or more proc behaviors, though some wizardry would be needed in terms of requesting proc rates and whatever from them. I don't know which would work out to be easier to deal with in practice.

dazer commented 13 years ago

What I proposed is far from easy to pull: the avalanche_spell proc belongs better in our instance of ProcList (since that proc would be weapon independent), but I don't think the weapon object can/should comunicate with that. The easy implementation of this would be forcing the caller to include the avalanche_spell proc if the avalanche enchant is present.

While that'd be easy to write, I feel it's a lazy-patch (and would force to go back to get_weapon_ep() too). I'm more inclined to persue the more abstract ProcBehaviour thing, but as of now I'm not sure I'll be able to pull it off.

dazer commented 13 years ago

I haven't solved the issue, however, I'd like to share my progress: Firstly, I refactored the proc_data file a bit: (see https://github.com/dazer/ShadowCraft-Engine/tree/procbehaviours). I think this could be usefull to assess the multiple behaviour. See that it simply implements a set_behaviour toggle that changes the behavior attributes. Writing a ProcBehaviour class is a few lines of code away from this.

The main issues I run across when trying to implement any of this is that we are appending new attributes to the objects in question (mh_only, proc_rate, etc.) in the model so, if we want to keep doing that, we'd need to instantiate two objects regardless of the approach.

The ProcBehaviour class has another problem: all the methods such as is_ppm() would belong to that new class, hence we'd need to point the callers to proc objects towards the behaviour object. Ultimately the easy solution is to pass the proc values (duration, name and such) to the behaviour too. This, I believe, is somewhat counterintuitive; besides it runs into the same problem described above.

Creating a hurricane proc in the model with 'default' behaviour (if any of the two weapons has the enchant) and appending it to the active_procs list seems to create a reasonable output. So, technicaly, this change offers the modeler the two flavors of the enchants but it doesn't finaly solve the problem, as those procs should be instantiated somewhere else if this is to be elegant in any fashion.

Edit: is importing deepcopy and duplicating the proc in the model elegant enough?