search

OverlayPlugin / cactbot

FFXIV TypeScript Raiding Overlay
Apache License 2.0
95 stars 35 forks source link

Why was `resources/effect_id.ts` significantly modified in the commit `resources:ko 6.35`? #280

Closed AsterOcclu closed 2 months ago

AsterOcclu commented 2 months ago

What's your question?

When I was developing the relevant plugin, I found that resources/effect_id.ts was significantly modified in https://github.com/OverlayPlugin/cactbot/commit/6c3dbeff549da5203cb5bb12987ddd8df6a59b91

Resulting in the deletion of codes such as 'NoMercy': '727', etc.

The title of this code commit is resources: ko 6.35. And I don't know why effect_id.ts was significantly modified?

Is this a bug or not?

image

valarnin commented 2 months ago

As is indicated at the top of the file, effect_id.ts is an auto-generated file. This file is generated based on XIVAPI data.

Because the file is a mapping of Name to ID, and there are status effects/effect IDs which have duplicate names, when those are encountered they need to be manually handled. This is done in gen_effect_id.ts's knownMapping table.

It appears that at some point, No Mercy and a number of other status effects had duplicate entries added, which were not handled properly. Running npm run util -- generate -f effect_id -ll info shows:

Info: Collision detected: NoMercy (IDs: 3042, 1831).  Skipping...

Given that we're now up to 515 collisions (and oof, that's a lot), it may be time to rethink how that file is structured in general.

Echoring commented 2 months ago

For this I have some thought.

  1. Skip collision names are not a good idea. The effects that easy to collision is usually famous and important effect. Due to their importance they are more likely to appear in NPC combatant's buff and PVP buff (e.g. the Balance). It may be better to keep all collision effect by adding its ID to name, like Embolden1297 and Embolden1239. One who want to use this will find which is effective himself.
  2. Effect can change name with patches, which may cause puzzling when handle CN/KO compatibility. (For example, at 6.0 DNC have Flourishing Flow, generated by both Fountain and Flourish, and at 6.1, to avoid overwriting, effect Flourishing Flow only generated by Fountain and change its name to Silken Flow, while a new effect Flourishing Flow generated by Flourish added. This makes a lot of puzzling.) It may be better to keep a previous version of effect_id, and load them based on version.

EDIT: ...I have a closer look and find that only jobs module is using EffectID, other module are all hard coding ID. Only 89 EffectID are referenced, and 38 of them have collisions, near a half. Maybe it's better to maintain it in constants rather than in gen_effect_ID.

MaikoTan commented 2 months ago

I think it might make sense to have a "base" id map with several "overlay layers", overriding parts of names based on the current game client version. And of course, the "base" one keeps the intl version as before. This method is still not good to handle collisions inside one version (like Dancer's Technical Step effect), but avoiding having Action and Action6x at the same time.

Echoring commented 2 months ago

This should be solved by #297.