rushanshekar / gen6-damage-calc

VGC-focused Damage Calculator for Gen 6 @ http://nuggetbridge.com/damagecalc
MIT License
0 stars 3 forks source link

buildDescription is being called an incredibly large number of times #53

Closed Tapin42 closed 9 years ago

Tapin42 commented 9 years ago

It would appear that buildDescription is being called a rather impressive number of times -- it looks like we've got extra change events happening, but I haven't gone much further than this right now.

Scenario Calls to buildDescription
On load (Common Showdown Mega Abom mirror match) 272
Change first Mega Abom to Common Showdown Mega Kang 160
Set Mega Kang's Atk to +1 12
Change Mega Kang to Common Showdown Mega Absol 144

So there's an optimization available here -- the third one there seems fairly close to reasonable (minimum reasonable number, IMO, would be 8 -- one for each attack. Of course, it'd be possible to go nuts and only recompute the ones that would actually be impacted by a stat change (dropping the minimum to zero), but I'm not even considering that yet).

augmt commented 9 years ago

What's the issue?

As the calculator is now, each Pokemon's set-selector is responsible for 17 calls to calculate(), and if you do the math those calls account for the 272 calls to buildDescription on load.

I'll count off the calls to calculate instigated from the set-selector's change event handler in order.

Caller # of calls to calculate()
.status.calc-trigger 2 (1 for each Pokemon)
.move.calc-trigger 4 (1 for each move)
.base.calc-trigger 5 (1 for each stat except hp)
.ability.calc-trigger 1
.item.calc-trigger 1
.forme.calc-trigger 1
.ability.calc-trigger 1
.item.calc-trigger 1
.set-selector 1

What can we change?

What if we could make it so that elements with the calc-trigger class only call calculate() when changed by a human? We can do this by using the event.originalEvent property and event.stopImmediatePropagation().

If we call event.stopImmediatePropagation() in the handler functions whenever a code-instigated change occurs, then calculate() won't be triggered!

It's ugly (which is why I haven't made a pull request yet) but I inserted this block of code

if (event.originalEvent === undefined) {
    event.stopImmediatePropagation();
}

into the top of the .status, .item, .ability, .move, .forme, and each of the stat (except hp) change event handler functions. This cuts the total # of calls to calculate() to two and buildDescription to 16 on load!

Now what about the other scenarios?

Scenario Calls to buildDescription
On load (Common Showdown Mega Abom mirror match) 16
Change first Mega Abom to Common Showdown Mega Kang 12
Set Mega Kang's Atk to +1 12
Change Mega Kang to Common Showdown Mega Absol 8
How's that? Seems like a good start to me.
Tapin42 commented 9 years ago

Daaang. That looks awesome. Nice find!

augmt commented 9 years ago

I'm wondering if I can avoid writing that block of code for each handler function and only write it, say, once.

Tapin42 commented 9 years ago

I spent a bit of time researching stopImmediatePropagation (and stopPropagation) this morning and haven't found any "clean" way yet to avoid having hooks in each event handler; but I'll keep looking too.

augmt commented 9 years ago

How about this?

calculate() is a handler so its first parameter is guaranteed to be the event object. Instead of using stopImmediatePropagation before calculate() is called, we can break out of calculate() whenever a code-instigated change occurs like so:

if (event.originalEvent === undefined) {
    return;
}

The problem with this is event.originalEvent is also undefined when the event's target is the set-selector. But the solution is simple: just add another case to the conditional:

if (event.target.className.indexOf("set-selector") === -1 && event.originalEvent === undefined) {
    return;
}

The results are consistent with the updated "Scenario vs. Calls to buildDescription" table.