baaron4 / GW2-Elite-Insights-Parser

Binary parser for the .evtc files that arcdps generates after a boss encounter. This will generate a .html file where the results can be easily reviewed.
MIT License
133 stars 47 forks source link

Fix incorrect phase names due to filtering #880

Closed Zerthox closed 7 months ago

Zerthox commented 7 months ago

This fixes incorrect phase names for phases after a < 2s phase, which can theoretically occur for several encounters. The problem was that GetPhasesByInvul filtered < 2s phases already, resulting in encounter logic implementations assigning incorrect phase names since some of them assume all phases to be present.

EliphasNUIT commented 7 months ago

Do you know which fights we are talking about and the order of magnitude of the filtered phases? There has been occurrences in the past where buffs we use to determine phases could be dropped and regained on super small windows and that is why that duration check was introduced there. We could reduce it to something more reasonable as I don't think phases that are around 50/100ms long can be considered as legitimate phases while a phase with a duration of 1s could be.

Zerthox commented 7 months ago

In theory it can happen on every fight where we do phase names based on the index of phases returned by GetPhasesByInvul. I listed all of them below. It becomes extra problematic if the logic uses i % 2 to distinguish between boss and split phases. Boss phases can end up being named as if they were a split phase and the other way around. I marked encounters where this could theoretically happen with *. In practice it's only the encounters where it is possible to go < 2s in any phase. This should definitely be possible on MAMA, Siax, Artsariiv and I reckon it might be possible on some Raid/Strike bosses where you can have really fast splits (e.g. oneshotting adds with Bladesworn stack). For example Adina speedkills would trigger the bug if the Adina encounter logic didn't implement the Invulnerability phase logic manually.

Zerthox commented 7 months ago

I think filtering with a lower millisecond duration is the sensible thing to do.