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

Official GF names by default #4241

Merged

Conversation

mrgriffin
Copy link
Collaborator

@mrgriffin mrgriffin commented Mar 5, 2024

Use official GF type/species/move/item names by default, instead of the unofficial run-together names that have been the default until now. This is achieved by introducing some narrower fonts, allowing us to fit more characters into the same space; we dynamically scale down the font width as needed in UIs that display names (e.g. the Pokemon Summary Screen and Battle Interface).

Species
image image image image image image image image image image image image image image image image

Items
image image image image image image image image image

Moves
image image image image image

Abilities (already expanded by default)
image

Types
image image

Credits: @mrgriffin FONT_NARROWER tweaks, FONT_SHORT_NARROW/FONT_SMALL_NARROWER graphics, code; @ZnogyroP FONT_NARROWER graphics; @cfmnephrite idea.

Changelog:

  • If the hack has changed fonts—e.g. to the FrLg ones—then they probably want to create their own FONT_NARROWER, FONT_SMALL_NARROWER, and FONT_SHORT_NARROW.
  • If the hack has introduced more UIs that render names they probably want to use GetFontIdToFit—see this PR's changes for examples of that.
  • If the hack has introduced names in list menus they can use .fontIdMayNarrow = TRUE on the list template.
  • Types, species, moves, and items all have individual commits which enable support in the UI and turn on longer names by default. These can be reverted by downstream projects if they really want to.

Future Work:

  • Think about how we want to handle 12-character nicknames in parts of the save without spare padding (maybe use SaveBlock3?).
  • Consider disabling the unofficial names.
  • Consider having official GF descriptions for moves/abilities where they would fit using a narrower font.

Closes #4249, #4248, #3915.

@mrgriffin mrgriffin marked this pull request as draft March 5, 2024 08:18
@mrgriffin
Copy link
Collaborator Author

CF raised TM pocket in the bag and menus in PokéMarts that sell TMs as two other places to check before this can be un-drafted. If there are any other places, please raise them. Move relearner? Move tutor?

@Bassoonian
Copy link
Collaborator

I wonder if this needs a config - I can imagine this isn't within everyone's stilistical goals. I'd also argue that any character that exists in the normal font should exist in this font too; there's quite some people with translated hacks (I've definitely seen French, Spanish and German) that'd get understandably upset if their diacritics are missing here.

Some text messages with move names buffered in string vars may overflow, perhaps? Did we check contests? The HGSS dex (move lists, but also move-based evolutions)?

@mrgriffin
Copy link
Collaborator Author

CF raised TM pocket in the bag and menus in PokéMarts that sell TMs as two other places to check before this can be un-drafted. If there are any other places, please raise them. Move relearner? Move tutor?

TMs in the mart work fine:
image

TMs in the bag need dynamic font widths:
image

Move relearner needs dynamic font widths:
image

Move tutor is fine, just goes via the normal learn screen:
image

@mrgriffin
Copy link
Collaborator Author

I wonder if this needs a config - I can imagine this isn't within everyone's stilistical goals.

I don't think it should require a separate one from B_EXPANDED_MOVE_NAMES, unless somebody wants to argue that a user prefers their text clipping to it dynamically resizing. I feel like either users want it to fit, or want it to resize.

I'd also argue that any character that exists in the normal font should exist in this font too; there's quite some people with translated hacks (I've definitely seen French, Spanish and German) that'd get understandably upset if their diacritics are missing here.

I think that's doable. From a quick glance over the font, I think the diacritics should mostly fit in 3px. Notable exceptions Ñ and whatever the R-looking thing is to the right of it.

Some text messages with move names buffered in string vars may overflow, perhaps?

Possibly. Is there a good way to check for that?

Did we check contests?

No, and they'll definitely need dynamic resizing (same space as the battle UI).
image

The HGSS dex (move lists, but also move-based evolutions)?

Ughhh. How does this screen work again?

@mrgriffin
Copy link
Collaborator Author

Just leaving a comment here: people may expect species names to get the same treatment. I think this would require a FONT_SMALL_NARROWER for the HP boxes, and is not something I currently plan on doing. It would be helpful to have a list of everywhere that a species name could be displayed.

If somebody else would like to tackle species names, they're welcome to contribute to this PR (or raise a separate PR) :)

@LOuroboros
Copy link
Collaborator

I have a small idea which I guess is in line with Jasper's suggestion above.

How about adding a function that takes an original font label as a parameter (Ex GetNarrowFont(u8 fontId)) that would return either fontId or FONT_NARROWER, and would be used in places like BattlePutTextOnWindow and PrintMoveOnWindow?

A function that would allow a user to quickly disable the usage of the narrower font if they so chose to basically.

@mrgriffin
Copy link
Collaborator Author

mrgriffin commented Mar 5, 2024

Why would anybody want to disable the narrower font, thus causing their text to clip?

I can imagine it being helpful to say "I'd rather the strings be shorter, so they don't exceed the box size" (which is what B_EXPANDED_MOVE_NAMES being off does), but I can't imagine it being helpful to say "I'd like the strings to get clipped against the box size".

I'm planning on having a function that would technically give a single place that lists all the fonts to test when dynamically narrowing fonts, so it's not that it would be hard to implement a config item... I just don't see how there's a use-case for having one.

I guess somebody else can come PR a config later if they really want to.

@LOuroboros
Copy link
Collaborator

I can imagine it being helpful to say "I'd rather the strings be shorter, so they don't exceed the box size"

It's exactly because of that.

I'm not going to sugarcoat it, this doesn't look good to me, I find it hard to read, so as a user I know that I would like to have a quick way to disable it:

310026517-e40eb3ae-057c-4e54-8601-cdc4e2f8f0f5

which is what B_EXPANDED_MOVE_NAMES being off does

Ah. If B_EXPANDED_MOVE_NAMES == FALSE disables this narrower font then please ignore what I said and carry on 👍

@mrgriffin
Copy link
Collaborator Author

Ah. If B_EXPANDED_MOVE_NAMES == FALSE disables this narrower font then please ignore what I said and carry on 👍

It doesn't exactly disable the narrower font, but because we use the widest font which will fit all the letters, the narrower font should never be used (unless there are shorter move names that currently clip, but I don't think that's the case?)

I think we're in agreement, but just for anybody else coming along—some of the letters being cut off looks like crap, so I don't think "the narrower font looks crap" is a good enough argument for a config item that chooses to cut letters off.

@LOuroboros
Copy link
Collaborator

Ah. If B_EXPANDED_MOVE_NAMES == FALSE disables this narrower font then please ignore what I said and carry on 👍

It doesn't exactly disable the narrower font, but because we use the widest font which will fit all the letters, the narrower font should never be used (unless there are shorter move names that currently clip, but I don't think that's the case?)

I think we're in agreement, but just for anybody else coming along—some of the letters being cut off looks like crap, so I don't think "the narrower font looks crap" is a good enough argument for a config item that chooses to cut letters off.

I didn't express myself clearly enough and I also didn't understand the effect of the changes, so that was entirely my bad.

What I feared was to have that ugly Stomping Tantrum. Now, if the narrower font is only ever used based on the width of the total amount of glyphs meaning that compact move names would use the regular font as usual, then I have no complaints and my suggestion was pointless from the start. I apologize, it was my bad :)

@mrgriffin
Copy link
Collaborator Author

mrgriffin commented Mar 5, 2024

Did we check contests?

No, and they'll definitely need dynamic resizing (same space as the battle UI).

Turns out they actually have even less space! Closer to 59 pixels than the battle UI's 64. Clips even with the narrower font, and even with even more aggressive kerning* on the i and T we'd still only save 2 pixels, meaning we'd blow the budget by a pixel.
image

* Which is probably an invasive implementation because it would mean altering RenderText and GetStringWidth to incorporate whatever kerning algorithm we settle on.

@mrgriffin
Copy link
Collaborator Author

TMs in the bag need dynamic font widths

Turns out these also are a pixel too narrow for Stomping Tantrum:
image

@mrgriffin
Copy link
Collaborator Author

Added support for contests and TM bag pockets. Have not yet added support for the move relearner because that uses a generic list menu and so the change is going to be a little more invasive.

If somebody did desperately want to reintroduce clipping they could stub out GetFontIdToFit, or they could alter sNarrowerFontIds to exclude the fonts they don't like (e.g. do [FONT_NARROW] = -1, to exclude FONT_NARROWER).

Note that there are failing tests because some move names do clip in contests, e.g. First Impression (only the first failure is reported).

PCG06 added a commit to PCG06/pokeemerald that referenced this pull request Mar 5, 2024
@mrgriffin
Copy link
Collaborator Author

Added move relearner, and also the ability to do the same font narrowing on any list template with .fontIdMayNarrow = TRUE (only enabled for the move relearner, but users might want it for other things too).

image

The remaining questions are:

  1. Does HGSS Pokédex need changing? Probably, but I haven't checked yet.
  2. Are there any other places where move names are shown?
  3. Are we okay with long names clipping in contests? Alex, Jasper, and myself think so.

@mrgriffin mrgriffin marked this pull request as ready for review March 5, 2024 13:09
@mrgriffin
Copy link
Collaborator Author

Does HGSS Pokédex need changing? Probably, but I haven't checked yet.

It mostly doesn't.
image
image

Very slight clipping on a 3rd stage evolution:
image

@mrgriffin
Copy link
Collaborator Author

mrgriffin commented Mar 5, 2024

Ready for review. I'll rebase this before it's merged.

I introduced PARAMETRIZE_LABEL to help debug failing regular tests, and added it to test/species.c too.

A FONT_SMALL_NARROWER would be a very helpful addition in the future—it helps us clean up theoretical edge cases in the HGSS Pokédex, paves the way for long species names by default, and provides a solution to #4243.

@mrgriffin mrgriffin force-pushed the rhh-narrower-font-long-move-names branch from 1e4540d to ef52486 Compare March 5, 2024 17:39
@mrgriffin
Copy link
Collaborator Author

mrgriffin commented Mar 5, 2024

Rebased. Hopefully nobody has any review comments, lol!

E: Unfortunately they did. So I need to rebase this again, no biggie! :)

Choose a reason for hiding this comment

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

Minor thing but would you be willing to remove the whitespace after "i"?

Copy link
Collaborator Author

@mrgriffin mrgriffin Mar 6, 2024

Choose a reason for hiding this comment

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

It's fine in the summary screen, but I found it really tricky to notice the i in things like ain without that space, to my eyes the dot looks like a diacritic on the n:
image

But other people can feel free to weigh in on that issue.

E: Also we need to make some trade-offs with some of the is that have diacritics (í, ì, ï, î), should those be 3px wide, or stay at 4px? And if shrunk, how?

@AlexOn1ine
Copy link
Collaborator

do we want this for 1.8 or 1.9?

@mrgriffin
Copy link
Collaborator Author

mrgriffin commented Mar 6, 2024

do we want this for 1.8 or 1.9?

If 1.9, might be worth having a FONT_SMALL_NARROWER and rolling this change out to species names, item names, and ability names.

I think it makes sense to let this cook for a release cycle on upcoming, just in case there are places that were missed. It doesn't seem as urgent now, due to other changes for 1.8.

@mrgriffin mrgriffin changed the title Long move names & narrower font Official GF move names Mar 6, 2024
@mrgriffin mrgriffin added this to the 1.9.0 milestone Mar 6, 2024
@mrgriffin mrgriffin marked this pull request as draft March 6, 2024 13:01
@mrgriffin
Copy link
Collaborator Author

Update: this is now targeting 1.9. My hope is to support official names for abilities, items, and species too.

@mrgriffin
Copy link
Collaborator Author

There we go, should be all ready to go now 🤞

@mrgriffin mrgriffin force-pushed the rhh-narrower-font-long-move-names branch from a6d0df0 to e0d13ca Compare April 25, 2024 06:30
@mrgriffin
Copy link
Collaborator Author

Rebased onto upcoming and did official type names while I was at it (see updated screenshots). These will make Terastalization a little nicer, saying "Fighting" rather "Fight", for example.

src/battle_main.c Show resolved Hide resolved
@AsparagusEduardo
Copy link
Collaborator

We're almost there xD

PARAMETRIZE_LABEL is like PARAMETRIZE, except that it allows the user to
provide a label which will be displayed in the test name line. This is
useful for tracking _which_ PARAMETRIZE case failed in the situation
where the numbers are unwieldy.
In contests, even FONT_NARROWER isn't sufficient to prevent clipping in
all cases. e.g. Stomping Tantrum clips. We have decided to accept that
cost to make the rest of the user experience better, but downstream
projects that don't like that trade-off can either a) alter the contest
UI, or b) set B_EXPANDED_MOVE_NAMES to FALSE.
In the Pokémon Storage System, even FONT_SMALL_NARROWER isn't sufficient
to prevent clipping in all cases. e.g. Unremarkable Teacup clips. We
have decided to accept that cost to make the rest of the user experience
better, but downstream projects that don't like that trade-off can
either a) alter the Pokémon Storage System UI, or b) set
I_EXPANDED_ITEM_NAMES to FALSE.
@mrgriffin mrgriffin force-pushed the rhh-narrower-font-long-move-names branch from 3e223a3 to 664fe90 Compare April 25, 2024 17:41
@mrgriffin
Copy link
Collaborator Author

Okay, fixed the missing HANDLE_EXPANDED_TYPE_NAMEs so that hopefully is the last change! 🤞

@AsparagusEduardo AsparagusEduardo merged commit dd098ba into rh-hideout:upcoming Apr 25, 2024
1 check passed
@pkmnsnfrn pkmnsnfrn mentioned this pull request Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants