Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: refactor splitter pt. 2 (auto-updater) #128

Merged
merged 12 commits into from
Apr 25, 2024
Merged

util: refactor splitter pt. 2 (auto-updater) #128

merged 12 commits into from
Apr 25, 2024

Conversation

wexxlee
Copy link
Collaborator

@wexxlee wexxlee commented Apr 7, 2024

Part 2 of the work on #94 (see #109 for more info).

This is built on the type-name changes in #127, so that should be merged first (and the changes in 94fd33e can be ignored, as I will rebase them out before merging).

This also sets both ActorControl and NameToggle to include: 'never'. Both are used ubiquitously in timelines (83 and 60 files, respectively) as wipe regex and to sync --untargetable-- and --targetable-- entries, and since those use cases are handled automatically by timeline tools, I wouldn't think they would have much utility via analysis filtering (although open to leaving them in if others feel differently?). I'm mostly disabling them from the filter in this PR to avoid massively cluttering the auto-generated PR that will follow. Speaking of which...

When this is merged, I expect an auto-generated PR to flag uses of (and attempt to add include: 'all' to):

  • RemovedCombatant (used in 3 triggers, 2 timelines)
  • NetworkCancelAbility (used in 2 timelines)
  • WasDefeated (used in 3 triggers, 1 timeline)
  • NetworkEffectResult (used in 1 trigger)
  • StartsUsingExtra (used in 2 triggers)

That said, I think we can save discussion & changes to those for the auto-generated PR.

@github-actions github-actions bot added docs /docs, /screenshots, *.md resources /resources raidboss /ui/raidboss module util /util ci /.github/ needs-review Awaiting review and removed raidboss /ui/raidboss module labels Apr 7, 2024
@valarnin
Copy link
Collaborator

valarnin commented Apr 9, 2024

This also sets both ActorControl and NameToggle to include: 'never'. Both are used ubiquitously in timelines (83 and 60 files, respectively) as wipe regex and to sync --untargetable-- and --targetable-- entries, and since those use cases are handled automatically by timeline tools, I wouldn't think they would have much utility via analysis filtering (although open to leaving them in if others feel differently?). I'm mostly disabling them from the filter in this PR to avoid massively cluttering the auto-generated PR that will follow. Speaking of which...

Any log lines that may be even tangentially related to triggers or timelines for a fight should be included, unless I'm misunderstanding what you're doing here. If a user uses the splitter to split a log file into individual encounters for playback in raidemulator, for example, all of the log lines that would be triggered on (including AC and NameToggle) should be included. This is also for purposes of e.g. testing timelines via the test_timeline.ts tool.

@wexxlee
Copy link
Collaborator Author

wexxlee commented Apr 9, 2024

Any log lines that may be even tangentially related to triggers or timelines for a fight should be included, unless I'm misunderstanding what you're doing here. If a user uses the splitter to split a log file into individual encounters for playback in raidemulator, for example, all of the log lines that would be triggered on (including AC and NameToggle) should be included. This is also for purposes of e.g. testing timelines via the test_timeline.ts tool.

I think there might be a misunderstanding. All log lines are included by default whenever a log is split. The analysisOptions property in netlog_defs controls only whether (and how many) of those log lines are included if the 'Filter Log for Analysis' checkbox is used in the log splitter tool (or the corresponding CLI option). The intent of that filter option was to (mostly) mirror the original regex that quisquous used to reduce a log file (or encounter) down to "interesting" lines to make it easier to create timelines & triggers. But that regex was based on ACT (not "network") lines, and it hadn't been updated to include a lot of the recently-added line types. The filter option was an effort to update and integrate that regex directly into the splitter tool.

The filter will (and should, I think) exclude a fair number of lines that aren't likely to be of interest when analyzing a log file for that purpose. I wouldn't expect the filter to be the default for most users, and especially not for logs being used for debugging/raid emulator/etc. Its sole intent is to trim logs to make them more readable for those of us actively working on trigger/timeline development. Perhaps that could be clearer in the UI?

@xiashtra
Copy link
Collaborator

xiashtra commented Apr 9, 2024

Any log lines that may be even tangentially related to triggers or timelines for a fight should be included, unless I'm misunderstanding what you're doing here. If a user uses the splitter to split a log file into individual encounters for playback in raidemulator, for example, all of the log lines that would be triggered on (including AC and NameToggle) should be included. This is also for purposes of e.g. testing timelines via the test_timeline.ts tool.

I think there might be a misunderstanding. All log lines are included by default whenever a log is split. The analysisOptions property in netlog_defs controls only whether (and how many) of those log lines are included if the 'Filter Log for Analysis' checkbox is used in the log splitter tool (or the corresponding CLI option). The intent of that filter option was to (mostly) mirror the original regex that quisquous used to reduce a log file (or encounter) down to "interesting" lines to make it easier to create timelines & triggers. But that regex was based on ACT (not "network") lines, and it hadn't been updated to include a lot of the recently-added line types. The filter option was an effort to update and integrate that regex directly into the splitter tool.

The filter will (and should, I think) exclude a fair number of lines that aren't likely to be of interest when analyzing a log file for that purpose. I wouldn't expect the filter to be the default for most users, and especially not for logs being used for debugging/raid emulator/etc. Its sole intent is to trim logs to make them more readable for those of us actively working on trigger/timeline development. Perhaps that could be clearer in the UI?

As long as the UI is clear to an end user which options are meant for splitting a log to send to us for debugging vs which options are meant more for developers I think it should be ok.

Copy link
Collaborator

@valarnin valarnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to keep complaining about parsing files with regex, but every time I see new regex to parse a file that's already being handled otherwise elsewhere, from my perspective it's another potential break point that can't easily be detected if there's any changes to the file format or other refactoring in the future.

Also, this.

https://stackoverflow.com/a/1732454

util/update_logdefs.ts Outdated Show resolved Hide resolved
util/update_logdefs.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the needs-review Awaiting review label Apr 21, 2024
@github-actions github-actions bot added raidboss /ui/raidboss module needs-review Awaiting review labels Apr 24, 2024
@wexxlee
Copy link
Collaborator Author

wexxlee commented Apr 24, 2024

OK, this updates the script to remove the regex dependency for trigger and timeline logic per earlier discussion. Separate commits for each to help with reviewability. With this change, it also became fairly trivial to add oopsy trigger checking as well, so I added that in dc14878.

(Note that by adding oopsy, the auto-generated PR that will follow after merging this will now propose to add NetworkDoT to the analysis filter as well.)

@wexxlee wexxlee requested a review from valarnin April 24, 2024 06:52
ui/raidboss/timeline_parser.ts Outdated Show resolved Hide resolved
util/update_logdefs.ts Outdated Show resolved Hide resolved
util/update_logdefs.ts Show resolved Hide resolved
@github-actions github-actions bot removed the needs-review Awaiting review label Apr 24, 2024
@github-actions github-actions bot added the needs-review Awaiting review label Apr 25, 2024
Copy link
Collaborator

@valarnin valarnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tentative pre-approval pending the discussion in the previous review about updating the comment.

@github-actions github-actions bot removed the needs-review Awaiting review label Apr 25, 2024
@wexxlee
Copy link
Collaborator Author

wexxlee commented Apr 25, 2024

Tentative pre-approval pending the discussion in the previous review about updating the comment.

Going to hold on merging this in case you want to take another look, since my last commit is a different approach than we talked about.

@valarnin
Copy link
Collaborator

Looks good to me now.

@wexxlee wexxlee merged commit 0f4ebe4 into OverlayPlugin:main Apr 25, 2024
15 checks passed
github-actions bot pushed a commit that referenced this pull request Apr 25, 2024
Part 2 of the work on #94 (see #109 for more info).

~~This is built on the type-name changes in #127, so that should be
merged first (and the changes in `94fd33e` can be ignored, as I will
rebase them out before merging).~~

This also sets both `ActorControl` and `NameToggle` to `include:
'never'`. Both are used ubiquitously in timelines (83 and 60 files,
respectively) as wipe regex and to sync --untargetable-- and
--targetable-- entries, and since those use cases are handled
automatically by timeline tools, I wouldn't think they would have much
utility via analysis filtering (although open to leaving them in if
others feel differently?). I'm mostly disabling them from the filter in
this PR to avoid massively cluttering the auto-generated PR that will
follow. Speaking of which...

When this is merged, I expect an auto-generated PR to flag uses of (and
attempt to add `include: 'all'` to):
  - `RemovedCombatant` (used in 3 triggers, 2 timelines)
  - `NetworkCancelAbility` (used in 2 timelines)
  - `WasDefeated` (used in 3 triggers, 1 timeline)
  - `NetworkEffectResult` (used in 1 trigger)
  - `StartsUsingExtra` (used in 2 triggers)

That said, I think we can save discussion & changes to those for the
auto-generated PR. 0f4ebe4
@wexxlee wexxlee deleted the logdef-auto-update branch April 25, 2024 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci /.github/ docs /docs, /screenshots, *.md raidboss /ui/raidboss module resources /resources util /util
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants