Skip to content

Support localized mutation point percentages#6948

Open
RyuDanuer wants to merge 4 commits intoRevolutionary-Games:masterfrom
RyuDanuer:ryudanuer/fix-6584-mutation-points-percentage
Open

Support localized mutation point percentages#6948
RyuDanuer wants to merge 4 commits intoRevolutionary-Games:masterfrom
RyuDanuer:ryudanuer/fix-6584-mutation-points-percentage

Conversation

@RyuDanuer
Copy link
Copy Markdown
Contributor

Brief Description of What This PR Does

This PR makes the mutation points bar support the full PERCENTAGE_VALUE formatting string instead of only showing the percent sign when it appears at the end.

I split the localized percentage format around {0} and place the prefix before the current mutation point value and the suffix after the base mutation point value. This lets formats like %{0} work while keeping the existing separate labels / arrow layout for pending mutation point changes.

I also made the bar refresh its cached percentage format parts when the active translation changes.

Manual testing:

  • I manually opened the mutation points bar scene in Godot using Turkish (--language tr) to check that the bar still loads and renders correctly.
  • Screenshot from that run: .test-artifacts/issue-6584-mp-bar-tr-2.png

I also ran:

dotnet build Thrive.csproj -v minimal
# 0 warnings, 0 errors

dotnet test test\code_tests\ThriveTest.csproj --no-build --no-restore -v minimal
# Passed: 100, Failed: 0

Related Issues

Closes #6584

Progress Checklist

Note: before starting this checklist the PR should be marked as non-draft.

  • PR author has checked that this PR works as intended and doesn't
    break existing features:
    https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
    (this is important as to not waste the time of Thrive team
    members reviewing this PR). This includes gameplay testing by the PR author.
  • Initial code review passed (this and further items should not be checked by the PR author)
  • Functionality is confirmed working by another person (see above checklist link)
  • Final code review is passed and code conforms to the
    styleguide.

Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.

@hhyyrylainen
Copy link
Copy Markdown
Member

Could you put the screenshot here on Github to show the test result?

@RyuDanuer
Copy link
Copy Markdown
Contributor Author

issue-6584-mp-bar-tr-2 Here.

@hhyyrylainen
Copy link
Copy Markdown
Member

But isn't that incorrect? In Turkish the percentage sign must be before the number. So that's basically the whole root problem of #6584

And in the image your percentage sign is after the number.

@RyuDanuer
Copy link
Copy Markdown
Contributor Author

I just tested it wrong before and posted the wrong screenshot. Sorry. Code is correct and I will soon commit to fix compilation issues..the Turkish translation is %{0}, but my first screenshot was from the standalone bar default state, so it didn’t actually go through UpdateMutationPoints() and wasn’t validating this PR properly.
issue-6584-mp-bar-tr-prefix-crop

Relevant test log:

locale: tr
percentage translation: %{0}
current mp label: %100
base mp label: / 100
saved screenshot result: 0

Copy link
Copy Markdown
Member

@hhyyrylainen hhyyrylainen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the screenshot it does look like it works, but the code change is extremely messey in the way it splits the format string.

I believe that it will be much more maintainable to have multiple translation keys and formats for all of the different combined parts. That way the translators will be able to make whatever is needed for their language.

Copy link
Copy Markdown
Member

@hhyyrylainen hhyyrylainen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You either need to put a lot more effort into making your PRs fit into the Thrive architecture and code style, or start picking purely small bugs that don't need any code refactoring as otherwise I will just stop reading these really badly made PRs that take way longer for me to read multiple times than to just do the fix myself.

Comment thread locale/en.po
Comment on lines +4303 to +4318
msgstr "{0}"

msgid "MUTATION_POINTS_CURRENT_WITH_PERCENTAGE"
msgstr "{0}%"

msgid "MUTATION_POINTS_CURRENT_WITH_RESULT"
msgstr "{0}"

msgid "MUTATION_POINTS_CURRENT_WITH_RESULT_WITH_PERCENTAGE"
msgstr "{0}%"

msgid "MUTATION_POINTS_RESULTING"
msgstr "{0}"

msgid "MUTATION_POINTS_RESULTING_WITH_PERCENTAGE"
msgstr "{0}%"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are literally all totally pointless because they are either the equivalent of a value to string conversion, or the percentage format.

Also you have directly edited this en.po file without using our localization script which is absolutely required when adding text.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, here is a documentation on how to work with Thrive's translation system. The system automatically adds these entries based on what translation tokens are present in the code.

freebuildingText = Localization.Translate("FREEBUILDING");
}

public override void _Notification(int what)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check if this was necessary?

There's already code in editors that should re-trigger the mutation points bar build.

See: UpdateMutationPointsBar called in CellEditorComponent.OnTranslationsChanged.

This is coded extremely sloppy. This is the kind of AI output I absolutely do not want to read.

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

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

Implement full support for PERCENTAGE_VALUE in the mutation points bar

3 participants