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

raidemulator: Refactor CombatantState object #5387

Merged
merged 6 commits into from
May 23, 2023
Merged

raidemulator: Refactor CombatantState object #5387

merged 6 commits into from
May 23, 2023

Conversation

valarnin
Copy link
Collaborator

@valarnin valarnin commented May 9, 2023

This PR is in preparation for combatantMemory lines.

Because the state tracker will now need to track all possible PluginCombatantState properties, it no longer makes sense to track a limited set for most other types of lines and a full state for those specific lines.

As such, the custom properties on CombatantState have been removed in favor of just implementing the PluginCombatantState interface with the carried over addition of a targetable property to maintain functionality.

Additionally, logic for constructing and cloning states was greatly simplified to use Object.assign instead.

Some duplicate properties were removed from Combatant in favor of the tracked state entries in CombatantState, and associated locations were updated to instead read the initial combatant state's version of those properties. This should result in more correct behavior when getCombatants is used, because name changes are tracked properly now for example.

The vast majority of the added lines are in a new resources/world_id.ts file, required to translate ID <=> Name. Excluding that file, the initial commit for this PR contains 166 additions and 205 deletions.

This PR needs more testing before being merged.

  • I have only tested with US DC names, for example, so I'm not sure if world name lookup will work as expected for CN/KO/JP servers.
  • I have not tested for logs that may have weird values for some entries that would potentially result in parseFloat returning NaN.
  • I have only tested recent content (DSRU, P8S, and TOP). Some older content should probably be tested.

@cactbotbot
Copy link
Collaborator

cactbotbot commented May 9, 2023

@valarnin Thanks for your contribution! 🌵🚀

@valarnin valarnin changed the title Replace CombatantState custom properties with `PluginCombatantState… raidemulator: Refactor CombatantState in preparation for combatantMemory lines May 9, 2023
@valarnin valarnin changed the title raidemulator: Refactor CombatantState in preparation for combatantMemory lines raidemulator: Refactor CombatantState object May 9, 2023
resources/world_id.ts Outdated Show resolved Hide resolved
resources/world_id.ts Outdated Show resolved Hide resolved
ui/raidboss/emulator/data/CombatantState.ts Show resolved Hide resolved
@valarnin
Copy link
Collaborator Author

I've identified an annoying issue with state tracking that is going to require rewriting the logic for the state calculation a bit.

Basically, because ID, Name, Job, and Level are no longer required to be calculated at a top level, they're not present on states between the initial state and the state which they first are calculated on (exclusive), that fails when displaying party information as well as causing issues if getCombatants is called for some reason between those two states during analysis or playback.

valarnin added 2 commits May 14, 2023 20:01
Rework combatant state tracking to carry forward all calculated values properly
Change casing on world_id.ts types
@github-actions github-actions bot added the util label May 15, 2023
@valarnin
Copy link
Collaborator Author

Issues are resolved, the automatic generator for world_id.ts has been added. Ready for re-review if needed.

@quisquous
Copy link
Owner

This all looks good to me, thanks. Do you feel like it still needs more testing or is this good to land?

@valarnin
Copy link
Collaborator Author

There was a bug fix that didn't get committed properly, apparently. More jank with my weird setup using WSL 🙃

I think this is good to go now.

@quisquous quisquous merged commit 401a4e4 into quisquous:main May 23, 2023
github-actions bot pushed a commit that referenced this pull request May 23, 2023
This PR is in preparation for `combatantMemory` lines.

Because the state tracker will now need to track all possible
`PluginCombatantState` properties, it no longer makes sense to track a
limited set for most other types of lines and a full state for those
specific lines.

As such, the custom properties on `CombatantState` have been removed in
favor of just implementing the `PluginCombatantState` interface with the
carried over addition of a `targetable` property to maintain
functionality.

Additionally, logic for constructing and cloning states was greatly
simplified to use `Object.assign` instead.

Some duplicate properties were removed from `Combatant` in favor of the
tracked state entries in `CombatantState`, and associated locations were
updated to instead read the initial combatant state's version of those
properties. This should result in more correct behavior when
`getCombatants` is used, because name changes are tracked properly now
for example.

The vast majority of the added lines are in a new
`resources/world_id.ts` file, required to translate `ID` <=> `Name`.
Excluding that file, the initial commit for this PR contains 166
additions and 205 deletions.

This PR needs more testing before being merged.

- I have only tested with US DC names, for example, so I'm not sure if
world name lookup will work as expected for `CN`/`KO`/`JP` servers.
- I have not tested for logs that may have weird values for some entries
that would potentially result in `parseFloat` returning `NaN`.
- I have only tested recent content (DSRU, P8S, and TOP). Some older
content should probably be tested. 401a4e4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants