util: fix encounter start bug in make/test timeline scripts

[wexxlee]

(sorry for the git bork/closed PR - one more time...)

This PR should address the encounter sync drift referenced in quisquous#5635 and quisquous#6048, as well as the missing zone-seal sync referenced in quisquous#5716.

There were a couple of separate but related issues that I found:

1. encounter_tools had not (yet) been updated to use InCombat lines to start encounters, even though make_timeline is inserting an InCombat sync at the start of a new timeline.
2. For fights that do not have zone seals, encounter_tools would fall back on using playerAttackingMob or mobAttackingPlayer regex. While this mostly still worked (with minor drift issues), it was also counting faerie healing actions as the start of the fight. I confirmed this was the case with the log in issue 6048. I don't have logs for the original report from issue 5635, but I suspect a similar cause there, as I was unable to repro in e6n when on non-pet classes.
3. I think there was a minor logic bug in encounter_tools re: pushing the fight-starting log line into logLines. When encountering certain log lines that should trigger a new fight encounter, onStartFight() would reinitialize this.currentFight and set .startTime; but when storeStartLine() was subsequently called, it would not push the starting log line into .logLines because the fight already had a start time set. This was causing make/test_timeline to sometimes not have access to (and not be able to sync on) the log line that started the encounter.

I'm wary about unintentional breakage, given the various different events that should (or should not) start a timeline. cc: @xiashtra and @JLGarber, would appreciate an extra set of eyes.

[JLGarber]

I'm not going to be able to test + review things properly tonight, unfortunately, but from the brief look I did this seems to be a reasonable fix. I can't remember why I didn't update encounter_tools alongside make_timeline, but you're right, those combined issues would definitely result in the behavior we've seen.

If Valarnin is able to review before I get to it tomorrow, that's great, otherwise I'll poke at this tomorrow afternoon and we'll see what we can see.

Thanks for tracking this down!

[JLGarber]

Thanks for taking the time to do all that! Apart from the question regarding removal of storeStartLine() from onNetSeal()I noted in-line, this looks very reasonable and worked fine on the logs I tested it on.

I think for fixing the faerie healing issue we might want to consider moving the ignoredCombatants definition to encounter_tools so we can reference it there to prevent this. I also noticed an apparently long-standing issue where overworld encounters are not consistently culled, but that seems to be unrelated to anything touched in this PR, so I will write up an issue for that later.

Let me know on the storeStartLine() thing and we'll get this merged then.

[wexxlee]

I think for fixing the faerie healing issue we might want to consider moving the ignoredCombatants definition to encounter_tools so we can reference it there to prevent this.

Agree. In theory we should never (?) be dropping to that code block if we're starting fights by seals & inCombat lines, but we might as well clean this up while we can. I'll tinker and push another commit to address this before we merge the PR.

I also noticed an apparently long-standing issue where overworld encounters are not consistently culled, but that seems to be unrelated to anything touched in this PR, so I will write up an issue for that later.

If you have a log and want to throw up an issue, I'm happy to tackle that next.

[JLGarber]

Agree. In theory we should never (?) be dropping to that code block if we're starting fights by seals & inCombat lines, but we might as well clean this up while we can. I'll tinker and push another commit to address this before we merge the PR.

Up to you. I'm fine merging this change as it is and handling the faerie issue separately, but if you want to consolidate it all here that's perfectly fine too. It maybe does make sense to have "stuff that fixes these bug symptoms" all contained in the one PR.

If you have a log and want to throw up an issue, I'm happy to tackle that next.

Yup, that would be fine, I'll try to write it up this weekend. I'll look at it At Some Point if you don't, it's not a high-priority issue.

[wexxlee]

Up to you. I'm fine merging this change as it is and handling the faerie issue separately, but if you want to consolidate it all here that's perfectly fine too. It maybe does make sense to have "stuff that fixes these bug symptoms" all contained in the one PR.

Added. Thx!