-
Notifications
You must be signed in to change notification settings - Fork 189
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
Generalize and localize standard practice section events #950
base: dev
Are you sure you want to change the base?
Generalize and localize standard practice section events #950
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the concept, but I'm not so sure about the current implementation of things.
There are also plenty of section names supported by Rock Band which aren't present within the RBN section name list, which we could consider adding down the line, but for now I think the current list will do.
|
||
if (isSectionNumber(remainder)) | ||
{ | ||
return Localize.Key("Gameplay.PracticeSections." + prcNameToLocalizationKey[key]) + " " + remainder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again here, the localization string should be a format string instead of manually constructed.
return Localize.Key("Gameplay.PracticeSections." + prcNameToLocalizationKey[key]) + " " + remainder; | |
return Localize.KeyFormat(("Gameplay.PracticeSections", prcNameToLocalizationKey[key], "WithNumber"), remainder); |
This would somewhat complicate the localization strings for each section, as both no-number and with-number variants would need to be provided for each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That no-number/with-number duplication would be a bummer. I think I have an idea for how to keep the names single-sourced while still allowing for different languages to take advantage of format strings for both numbered and unnumbered names, but I'm not sure if it still constitutes manually constructing localized strings.
The list of localizations for section names would remain unchanged, except "Section": "section"
would become "LetterSection": "{0} section"
(in English). We'd also add a new localization string, "WithNumber": "{0} {1}"
(again, English value for example - maybe another language would prefer "{1} {0}"
or whatever).
So if we have a section event with no number, we just localize it and that's it. If it does have a number, we localize the name (ignoring the number), then pass that localized value (name
) and unlocalized section number (number
) to Localize.KeyFormat("Gameplay.PracticeSections.WithNumber", name, number)
in order to render the full section name in a language-appropriate format.
Is that two-step localization okay? Both in general principle, and also in covering our bases for what different languages might need to account for with section numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we'd want to move LetterSection
and WithNumber
out of the same grouping as the actual section flags, to avoid false positives if someone actually uses something like [section WithNumber]
for some reason. Gameplay.Practice.Sections
for the literal flags and Gameplay.Practice.SectionFormats
for those two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a commit with this approach implemented - let me know what you think 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot about a couple more nitpicks I had lol, name casing of functions should generally be PascalCase regardless of visibility.
-Move initial parsing back to Core, making the logic prc/section-agnostic -Use snake case for localization tags -Dictionary now exists only to normalize typo'd/inconsistent names from the RBN docs -Construct keys by using tuple-based overloads -Use format strings for letter-based sections and sections with numbers -Non-eng localization files still have populated English values for now (pending question)
RB uses a static list of practice sections. YARG currently parses these events by transforming the contents of the events themselves, but this doesn't always result in the correct name. For example,
[prc_a1]
is meant to render as "A Section 1", but appears in YARG as "A1". Furthermore, using preset section names means that they can be localized, and the charting team has been sticking to these events as much as possible for that reason.One drawback of RB's system is that it has arbitrary limitations on how many times a section can be repeated (e.g. verses go up to 9, but pre-verses only go up to 5) and how many subsections a section can have (e.g. Guitar Solo 1 goes up to 1S, but Guitar Solo 9 only goes up to 9N).
This PR integrates the standard practice sections into the localization system, and generalizes them to remove the arbitrary limitations. Section numbers can be any integer with any number of digits - you can have Guitar Solo 345678 - while subsection letters have to be single-letter and are capped at Z. Special consideration is given to the "letter-based" sections (A Section, B Section, etc), which are also generalized all the way up to Z Section.
In all cases, the section number and subsection letter are preserved regardless of language setting, and it is only the proper name of the section that gets localized, like Guitar Solo, Breakdown, or Post-Chorus.
If a practice section fails to match any standardized tag, then it is parsed in the same way that the game currently parses all practice sections.