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

resources: Update netlog_defs filters (auto-generated) #135

Merged
merged 3 commits into from
Apr 29, 2024
Merged

Conversation

github-actions[bot]
Copy link

Caution

NOTE TO REVIEWERS:
Due to GitHub limitations, you must close & re-open this PR before merging to ensure pre-merge workflows (e.g. linters) are correctly triggered.**

This PR was auto-generated based on a recent repo commit to either resources/netlog_defs.ts or to a file in ui/raidboss/data.

It updates the analysis filter criteria to ensure all log line types currently being used by raidboss triggers/timelines are included in the log splitter's analysis filter (unless actively suppressed).

This update was triggered after finding the below uses of certain log line types. Please carefully review these uses to determine if including all log lines of these types in the analysis filter is appropriate.

You can instead change the include property to 'filter' (and add filters:), or to 'never' to suppress the log line type from the filter. Changes can be pushed to the PR branch before merging this PR.

RemovedCombatant
id: 'O5N Stop Combat',
type: 'RemovedCombatant',
netRegex: { name: 'Phantom Train', capture: false },
id: 'O5S Stop Combat',
type: 'RemovedCombatant',
netRegex: { name: 'Phantom Train', capture: false },
id: 'ShinryuEx Heart Cleanup',
type: 'RemovedCombatant',
netRegex: { name: 'Shinryu', capture: false },
0.0 "--Reset--" RemovedCombatant { name: "Proto Ozma" } window 0,100000 jump 0
196.0 "--sync--" RemovedCombatant { name: "Liquid Hand" } window 50,0

NetworkCancelAbility
3154.9 "--sync--" NetworkCancelAbility { id: "3878", source: "Raiden" } window 40,0
3341.7 "--sync--" NetworkCancelAbility { id: "3878", source: "Raiden" } window 40,0
993.1 "--sync--" NetworkCancelAbility { id: "63C3", source: "King Thordan" } window 300,0

NetworkDoT
id: 'DSR Pyretic',
type: 'NetworkDoT',
// Amazingly, the dot/hot line has the effect id for pyretic here. Most dots don't.

WasDefeated
id: 'Eureka Wraith Count',
type: 'WasDefeated',
netRegex: { target: 'Shadow Wraith', capture: false },
id: 'Shifting Altars of Uznair Altar Totem Flames of Fury Cleanup',
type: 'WasDefeated',
netRegex: { target: 'Altar Totem', capture: false },
id: 'DelubrumSav Queen Ball Lightning Bubble',
type: 'WasDefeated',
netRegex: { target: 'Ball Lightning', capture: false },
6000.0 "--Reset--" WasDefeated { target: "Dahu" } window 0,2000 jump 0

NetworkEffectResult
id: 'A11S Plasma Shield Shattered',
type: 'NetworkEffectResult',
netRegex: { name: 'Plasma Shield', currentHp: '0', capture: false },

StartsUsingExtra
id: 'UWU Ifrit Initial Radiant Plume Collector',
type: 'StartsUsingExtra',
netRegex: { id: '2B61' },
id: 'E8S Icicle Impact',
type: 'StartsUsingExtra',
netRegex: { id: '4DA0' },

Caution

REMINDER:
Please don't forget to close & re-open this PR before merging to ensure pre-merge workflows are correctly triggered!

@valarnin valarnin closed this Apr 26, 2024
@valarnin valarnin reopened this Apr 26, 2024
@github-actions github-actions bot added resources /resources needs-review Awaiting review labels Apr 26, 2024
@wexxlee
Copy link
Collaborator

wexxlee commented Apr 26, 2024

FWIW, my thoughts on these:
(I haven't tested this yet, but I think if we decide to make changes, we can just commit directly to the update-logdefs branch without needing to merge+do a follow-up PR)

RemovedCombatant

I think ok to add but filtered only to NPC combatants (same as AddedCombatant) - e.g.

    analysisOptions: {
      include: 'filter',
      filters: { id: '4.{7}' }, // NPC combatants only
      combatantIdFields: 2,
    },

NetworkCancelAbility

Seems pretty edge case, but I guess could be useful for phase jumps? If so, should def be filtered to NPC combatants only.

NetworkDoT

I think this can probably just be suppressed (e.g. 'never')?

WasDefeated

I think OK to include if filtered to NPC combatants.

NetworkEffectResult

Another one I think could be suppressed.

StartsUsingExtra

I think it should be included but filtered to NPC combatants to match the filter on 0x14 lines.

@valarnin
Copy link
Collaborator

I agree with most of these, except:

NetworkDoT

I think this can probably just be suppressed (e.g. 'never')?

I think this should be included but filter to only lines with effectId: '[^0][^\']*?' or something else indicating a valid effectId.

(realistically I would prefer to filter to both DoT && valid effectId but the filtering is || instead)

For numbers, across 33,289,262 lines dating back to 2024-01-08, 1,069,726 (3.2%) of those are NetworkDoT lines.
Across those 1,069,726 NetworkDoT lines, 785,262 (73.4%) of those have an effectId of 0, and 284,464 (26.6%) have a valid effectId.
So the rate of inclusion would be 0.85% of all lines.

@wexxlee
Copy link
Collaborator

wexxlee commented Apr 26, 2024

NetworkDoT

I think this can probably just be suppressed (e.g. 'never')?

I think this should be included but filter to only lines with effectId: '[^0][^\']*?' or something else indicating a valid effectId.

(realistically I would prefer to filter to both DoT && valid effectId but the filtering is || instead)

Filtering can be && - if both field filters are in the same object, it's &&; if a different object, it's ||. Good example of this is on the logdef for 0x1A:

    analysisOptions: {
      include: 'filter',
      filters: [
        { // effect from environment/NPC applied to player
          sourceId: '[E4].{7}',
          targetId: '1.{7}',
        },
        { // known effectIds of interest
          effectId: ['B9A', '808'],
        },
      ],
      combatantIdFields: [5, 7],
    },

For numbers, across 33,289,262 lines dating back to 2024-01-08, 1,069,726 (3.2%) of those are NetworkDoT lines. Across those 1,069,726 NetworkDoT lines, 785,262 (73.4%) of those have an effectId of 0, and 284,464 (26.6%) have a valid effectId. So the rate of inclusion would be 0.85% of all lines.

Would it make sense to further filter id to playerIds (1.{7}), so we only pick up DoTs on players?

@valarnin
Copy link
Collaborator

Would it make sense to further filter id to playerIds (1.{7}), so we only pick up DoTs on players?

I'm ambivalent. We don't currently have any potential use for a DoT on an enemy but that might not always be the case in the future.

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.

Looks good aside from the potential regex issue.

resources/netlog_defs.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the needs-review Awaiting review label Apr 28, 2024
@wexxlee wexxlee merged commit b919029 into main Apr 29, 2024
9 checks passed
@wexxlee wexxlee deleted the update-logdefs branch April 29, 2024 02:31
github-actions bot pushed a commit that referenced this pull request Apr 29, 2024
#135)

> [!CAUTION]
> ***NOTE TO REVIEWERS:***
> Due to GitHub limitations, you ***must*** close & re-open this PR
before merging to ensure pre-merge workflows (e.g. linters) are
correctly triggered.**

This PR was auto-generated based on a recent repo commit to either
`resources/netlog_defs.ts` or to a file in `ui/raidboss/data`.

It updates the analysis filter criteria to ensure all log line types
currently being used by raidboss triggers/timelines are included in the
log splitter's analysis filter (unless actively suppressed).

This update was triggered after finding the below uses of certain log
line types. Please carefully review these uses to determine if including
all log lines of these types in the analysis filter is appropriate.

You can instead change the `include` property to `'filter'` (and add
`filters:`), or to `'never'` to suppress the log line type from the
filter. Changes can be pushed to the PR branch before merging this PR.

## `RemovedCombatant`
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/04-sb/raid/o5n.ts#L17-L19
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/04-sb/raid/o5s.ts#L17-L19
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/04-sb/trial/shinryu-ex.ts#L20-L22
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/04-sb/eureka/eureka_hydatos.txt#L13
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/05-shb/ultimate/the_epic_of_alexander.txt#L49
## `NetworkCancelAbility`
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/04-sb/eureka/eureka_hydatos.txt#L139
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/04-sb/eureka/eureka_hydatos.txt#L161
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/06-ew/ultimate/dragonsongs_reprise_ultimate.txt#L156
## `NetworkDoT`
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/oopsyraidsy/data/06-ew/ultimate/dragonsongs_reprise_ultimate.ts#L280-L282
## `WasDefeated`
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/04-sb/eureka/eureka_anemos.ts#L157-L159
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/04-sb/map/the_shifting_altars_of_uznair.ts#L138-L140
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/05-shb/eureka/delubrum_reginae_savage.ts#L2507-L2509
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/05-shb/eureka/delubrum_reginae_savage.txt#L315
## `NetworkEffectResult`
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/03-hw/raid/a11s.ts#L349-L351
## `StartsUsingExtra`
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/04-sb/ultimate/ultima_weapon_ultimate.ts#L625-L627
https://github.com/OverlayPlugin/cactbot/blob/0f4ebe427c3be1608d64ced380814c0dd924902e/ui/raidboss/data/05-shb/raid/e8s.ts#L464-L466

> [!CAUTION]
> ***REMINDER:***
> Please don't forget to close & re-open this PR before merging to
ensure pre-merge workflows are correctly triggered!

---------

Co-authored-by: wexxlee <[email protected]> b919029
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resources /resources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants