raidboss: Add support for netregexes in timelines

[valarnin]

Closes #5145

Obviously needs plenty of testing etc.

Example added to test.txt timeline, can be triggered with /e testNetRegexTimeline.

Took the opportunity to refactor the TimelineParser.parse function to extract out some of the line matching, so there wasn't a need for a 30+-space-indented set of lines, and to allow reusing some of the logic for the rest of the line matching after the regex.

[quisquous]

Thank you for this! Overall, I think the implementation and refactoring looks great. I have a couple of comments about some edge cases around translations and tests, but that's it.

[quisquous]

Also cc @JLGarber here too for timeline thoughts

[quisquous]

Thanks!

I think I want to give this a little bit more testing and I want to give this a little bit of time for feedback from anybody else too before merging it, but overall this looks good to me and I think we should be able to auto-convert most things to this format.

[JLGarber]

Nothing to add here, this looks great. I'll look at updating make_timeline at some point in the future.

[quisquous]

A bunch of comments here.

Sorry about the conflict, it's due to the moved regex block and is just from #5940

Am I right in understanding the code that we're matching all net/parsed log lines against all net/parsed regexes? Do you think it'd be worth separating out "this is a netregex" and "this is a regex" and only matching particular lines against particular ones? It'd certainly prevent duplicating the amount of matching work going on.

One question I have is if you think it'd be worth supporting bareword keys (via an eval -> json stringify -> json parse), e.g. Ability { id: "8A03", source: "Eulogia" } which I find a little easier on the eyes but maybe that's just me having read too much javascript. Maybe that's a little too hacky. I think an eval is definitely a little bit sketchy too, but I think we're already evaling trigger and user code so it's not any worse.

Finally, I ran into some timeline test issues. I did a search and replace on sync / 1\[56\]:\[\^:\]\*:([^:]*):([^:]*):/ for Ability { "id": "$2", "source": "$1" }. I didn't do any further testing, but timeline test does not work here with this with either an fflogs or local file. All replaced lines are missed by the timeline test. Repro steps are:

(1) do that replace
(2) add a large window sync to the 14 line, e.g. 4009.6 "--sync--" sync / 14:[^:]*:Eulogia:8A03:/ window 10000,10
(3) run ts-node util/logtools/test_timeline.ts -t thaleia -k YOURKEYHERE -r wRA1yPgGx8McNjnf -rf 7

If I do add a 0 "--sync--" Ability { "id": "1D6D" } window 10000,10000 to the test timeline for provoke, that does start the timeline, so it's not a fundamental issue with non-gamelog lines in the timeline code and so I suspect that test_timeline doesn't know anything about netregex log lines.

[valarnin]

Sorry about the conflict, it's due to the moved regex block and is just from #5940

That's fine, I'll resolve it like I normally do (with a rebase/force-push) before I continue any other changes on this PR.

Am I right in understanding the code that we're matching all net/parsed log lines against all net/parsed regexes? Do you think it'd be worth separating out "this is a netregex" and "this is a regex" and only matching particular lines against particular ones? It'd certainly prevent duplicating the amount of matching work going on.

I'm assuming by this comment you're referring to this code, which just redirects NetLog to LogEvent?

https://github.com/valarnin/cactbot/blob/c8ba9bf79d4266c91b34b3cecd52fb3fa07abdf5/ui/raidboss/timeline.ts#L726-L733

I went for simplicity on this PR, rather than also trying to de-duplicate logic in OnLogEvent, as that would also entail adding additional support for netRegex equivalents of the regexes being checked there, as well as storing netRegex checks differently, etc. That would likely have entailed adding another 200+ lines of code to this PR, as a quick rough estimate.

It probably makes sense to make that a high-priority followup PR maybe, unless you're fine with adding a bunch of extra code to this PR?

One question I have is if you think it'd be worth supporting bareword keys (via an eval -> json stringify -> json parse), e.g. Ability { id: "8A03", source: "Eulogia" } which I find a little easier on the eyes but maybe that's just me having read too much javascript. Maybe that's a little too hacky. I think an eval is definitely a little bit sketchy too, but I think we're already evaling trigger and user code so it's not any worse.

I'm fine with handling this either way, it makes no difference to me, though a comment detailing the "why" for that sort of process would be required I think.

Finally, I ran into some timeline test issues. I did a search and replace on sync / 1\[56\]:\[\^:\]\*:([^:]*):([^:]*):/ for Ability { "id": "$2", "source": "$1" }. I didn't do any further testing, but timeline test does not work here with this with either an fflogs or local file. All replaced lines are missed by the timeline test. Repro steps are:

(1) do that replace (2) add a large window sync to the 14 line, e.g. 4009.6 "--sync--" sync / 14:[^:]*:Eulogia:8A03:/ window 10000,10 (3) run ts-node util/logtools/test_timeline.ts -t thaleia -k YOURKEYHERE -r wRA1yPgGx8McNjnf -rf 7

If I do add a 0 "--sync--" Ability { "id": "1D6D" } window 10000,10000 to the test timeline for provoke, that does start the timeline, so it's not a fundamental issue with non-gamelog lines in the timeline code and so I suspect that test_timeline doesn't know anything about netregex log lines.

I'll investigate this. I didn't think to touch test_timeline because the actual unit tests didn't fail and I didn't have any issues with in-game tests and raidemulator tests.

[quisquous]

Am I right in understanding the code that we're matching all net/parsed log lines against all net/parsed regexes? Do you think it'd be worth separating out "this is a netregex" and "this is a regex" and only matching particular lines against particular ones? It'd certainly prevent duplicating the amount of matching work going on.

I'm assuming by this comment you're referring to this code, which just redirects NetLog to LogEvent?

https://github.com/valarnin/cactbot/blob/c8ba9bf79d4266c91b34b3cecd52fb3fa07abdf5/ui/raidboss/timeline.ts#L726-L733

Yeah, that's exactly it.

I went for simplicity on this PR, rather than also trying to de-duplicate logic in OnLogEvent, as that would also entail adding additional support for netRegex equivalents of the regexes being checked there, as well as storing netRegex checks differently, etc. That would likely have entailed adding another 200+ lines of code to this PR, as a quick rough estimate.

It probably makes sense to make that a high-priority followup PR maybe, unless you're fine with adding a bunch of extra code to this PR?

I'm fine with extra code in this PR. I don't think you need to deduplicate OnLogEvent, though. My understanding here is that for backwards compatibility in some medium term sense, we'll need to send both types of log lines to the timeline. If that's true, then I think moving the code in OnLogEvent that's manually checking countdowns etc to netRegex could be done in a followup and a TODO could be left here.

What I'm really asking for is having a separate OnParsedLog / OnNetLog sort of path into TimelineController and into Timeline itself, and then a separate set of activeNetSyncs vs activeSyncs to check which probably requires a few more checks in _CollectActiveSyncs to put syncs in the right bucket.

[quisquous]

One question I have is if you think it'd be worth supporting bareword keys (via an eval -> json stringify -> json parse), e.g. Ability { id: "8A03", source: "Eulogia" } which I find a little easier on the eyes but maybe that's just me having read too much javascript. Maybe that's a little too hacky. I think an eval is definitely a little bit sketchy too, but I think we're already evaling trigger and user code so it's not any worse.

I'm fine with handling this either way, it makes no difference to me, though a comment detailing the "why" for that sort of process would be required I think.

Also as a clarification, this is more of a style question than an implementation one (maybe there's a better way). This definitely is more of a follow-up sort of fix either way.

I think really it's test_timeline and separating out the log handling that are the two pieces that I want to land with this PR.

[valarnin]

Rebased and force-pushed.

I think this addresses all the outstanding issues, but please verify because I did this while juggling 4 other things.

There were two issues in test_timeline, the fixes should be pretty obvious.

Added more TODOs to address what was discussed here.

Opted for a more simple splitting of syncs rather than a more thorough one for simplicity. Maybe someday I'll rewrite the entire Timeline codebase so it's less... dated.

[quisquous]

Thanks for all these changes. Looks good to me, and test_timeline.ts seems to work with both a file and fflogs with these fixes.

[quisquous]

Opted for a more simple splitting of syncs rather than a more thorough one for simplicity. Maybe someday I'll rewrite the entire Timeline codebase so it's less... dated.

Yeahhhhh, there's still a good bit of 2017/2018 code in there for sure.