cactbot icon indicating copy to clipboard operation
cactbot copied to clipboard

Remove BNPC names from files and use IDs instead

Open Akurosia opened this issue 1 year ago • 7 comments

Based on https://github.com/quisquous/cactbot/pull/5645#event-9764442697 (and to have a seperate topic available)

Current issue stated by quis: Unfortunately there's only bnpcid on the added combatant line but not on other lines.

Suggestions sofar:

quis: Other options here could be that the translation section could have a bnpcid -> name section (like the syncs, but just referenced differently) or alternatively alternatively we could consider dropping names from triggers (and timelines?)

xiashtra suggestion was to ask ravahn to includ bnpcid to all other loglines

I am currently unsure of following things:

  • can cactbot retrieve the names itself from game provided the ID or can we provid a file with a mapping (like we do it for a couple other stuff that needs to be generated from time to time)
  • can names be always dropped from triggers? i thing there will be cases were we still need names and could run into these problems

Akurosia avatar Jul 08 '23 15:07 Akurosia

  • can names be always dropped from triggers? i thing there will be cases were we still need names and could run into these problems

I can think of at least a few triggers that would break if we did that.

I also think it makes triggers/timelines less human readable with only ability IDs to go off of.

xiashtra avatar Jul 08 '23 18:07 xiashtra

Yeah, those are also my concerns as well re: removing names. I was just enumerating some options that have been brought up in the past for completeness.

quisquous avatar Jul 08 '23 23:07 quisquous

I think that a mapping file that was something like the following would be fine for readability? Could be auto-generated from SaintCoinach exports or via XIVAPI, whichever is easier to wire up?

const NpcMapping = {
  TiamatsClone: {
    en: 'Tiamat\'s Clone',
    jp: 'ティアマット・クローン',
    // etc
    bNpcNameId: 12242,
  },
};

Usage:

    {
      id: 'EO 21-30 Tiamat Clone Dark Wyrmwing',
      type: 'StartsUsing',
      netRegex: { id: '7C65', source: NpcMapping.TiamatsClone.en, capture: false },
      response: Responses.goMiddle(),
    },

The translation layer in translations.ts could then auto-translate these names, reducing the required boilerplate for timelineReplace as well?

We could probably do something similar for Actions.csv and auto-translate that too? Just spitballing ideas.

valarnin avatar Jul 11 '23 17:07 valarnin

That's a nice idea. I do worry some about collisions here, but maybe we could just append the name id in that case, e.g. TiamatsClone12242?

I also wonder if we could also support LocaleText in netRegex params as well, so it could just be NpcMapping.TiamatsClone (with no .en) and then no translation needed, since it's in the object.

The tricky part here is that we'd need to do something for timelines as well, and in some cases you can't auto-generate timelineReplace because of the collision requirements (e.g. look at how Alpha-Omega is specified in TOP). I added some more doc about collisions in RaidbossGuide.md recently too. So, we'd need some solution for the timeline end somehow, but maybe we could roll that into netRegex in timelines somehow.

quisquous avatar Jul 11 '23 19:07 quisquous

I think that a mapping file that was something like the following would be fine for readability? Could be auto-generated from SaintCoinach exports or via XIVAPI, whichever is easier to wire up?

If you're auto-generating these, what's to prevent the same problem of the NpcMapping.[name] changing when there's a rename? What about when the rename is more than just a simple punctuation change (like we saw with Criterion in 6.28)? We can end up in a situation where the triggers refer to an old name that no longer exists, which makes maintenance confusing.

xiashtra avatar Jul 11 '23 19:07 xiashtra

I think that a mapping file that was something like the following would be fine for readability? Could be auto-generated from SaintCoinach exports or via XIVAPI, whichever is easier to wire up?

If you're auto-generating these, what's to prevent the same problem of the NpcMapping.[name] changing when there's a rename? What about when the rename is more than just a simple punctuation change (like we saw with Criterion in 6.28)? We can end up in a situation where the triggers refer to an old name that no longer exists, which makes maintenance confusing.

Yeah, that's awkward. It doesn't happen that often though, and at the very least it will throw TypeScript errors in ~most cases so it should be obvious what needs fixing. That's more than we have now!

quisquous avatar Jul 11 '23 19:07 quisquous

Couldn't we handel bnpc (and maybe actions) like we do for petnames? and if we need to use them just refere to the ids oh the newly generated file? than we wouldn' need to specify enemies in the timeline files at all and just use the generated resources to get the localized name?

Akurosia avatar Jul 15 '23 11:07 Akurosia