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

Remove BNPC names from files and use IDs instead #5659

Open
Akurosia opened this issue Jul 8, 2023 · 7 comments
Open

Remove BNPC names from files and use IDs instead #5659

Akurosia opened this issue Jul 8, 2023 · 7 comments

Comments

@Akurosia
Copy link
Contributor

Akurosia commented Jul 8, 2023

Based on #5645 (comment) (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
@xiashtra
Copy link
Contributor

xiashtra commented Jul 8, 2023

  • 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.

@quisquous
Copy link
Owner

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.

@valarnin
Copy link
Collaborator

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.

@quisquous
Copy link
Owner

quisquous commented Jul 11, 2023

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.

@xiashtra
Copy link
Contributor

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.

@quisquous
Copy link
Owner

quisquous commented Jul 11, 2023

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!

@Akurosia
Copy link
Contributor Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants