Skip to content

Application: General i18n workaround for shared font loading - #76

Closed
CaiMiao wants to merge 2 commits into
natinusala:masterfrom
CaiMiao:master
Closed

CaiMiao wants to merge 2 commits into
natinusala:masterfrom
CaiMiao:master

Conversation

@CaiMiao

@CaiMiao CaiMiao commented Jan 20, 2021 •

Copy link
Copy Markdown

This is similar to #49 but I made a more graceful implement (at least imo), and I didn't aware of #49 when I was writing this lol

For memory consuming concern, we do not load full fallback chain when we are not in title takeover mode.

Confirmed working for my WIP tiny tool.

I'm not really a programmer so let me know if cleanup/optimization is needed.


Still I have a question, I can't really figure out how nvgAddFallbackFontId push the fallback order so I may have reversed my intend.

For example:

nvgAddFallbackFontId(Application::vg, Application::fontStash.regular, Application::fontStash.korean);
nvgAddFallbackFontId(Application::vg, Application::fontStash.regular, Application::fontStash.sharedSymbols);

does make the order regular->korean->symbol (appending) or regular->symbol->korean (prepending)?

Comment thread library/include/borealis/i18n.hpp Outdated
*/
std::string getCurrentLocale();

#ifdef __SWITCH__

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why put it here? Technically it's i18n related but it doesn't belong to the i18n system

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I mean it depends on the Switch implementation, and i18n is supposed to be generic (and eventually will)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this id only make sense for libnx i think
removed #ifdef

Comment thread library/lib/application.cpp Outdated
Logger::warning("Non applet mode, font full fallback is enabled!");
}

if (locale == 6 || locale == 15 || isFullFallback)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Not a fan of magic values here, could you define them somewhere instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

substituted with enforcing SetLanguage enum

Comment thread library/lib/application.cpp Outdated
if (at == AppletType_Application || at == AppletType_SystemApplication) // title takeover
{
isFullFallback = true;
Logger::warning("Non applet mode, font full fallback is enabled!");

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is this really a warning?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed to INFO

Comment thread library/lib/i18n.cpp Outdated
}
else
{
brls::Logger::error("Unable to convert system language ID (error 0x{0:x}), using the default one: {1}", res, DEFAULT_LOCALE);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You can use {0:#x} instead of 0x{0x:}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

@natinusala

Copy link
Copy Markdown
Owner

Thanks for the PR!

nvgAddFallbackFontId works by appending the fallback, so the order would be regular then Korean then shared symbols.

I am going to wait for you to add proper names to the locale magic values since I am having a real hard time to understand the second part of the code without it.

Comment thread library/lib/application.cpp Outdated
Logger::info("Using Switch shared font");
Application::fontStash.regular = Application::loadFontFromMemory("regular", font.address, font.size, false);
Logger::info("Adding Switch shared standard font");
Application::fontStash.standard = Application::loadFontFromMemory("regular", font.address, font.size, false);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Wouldn't that be standard as well in the loadFontFromMemory call?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed

@natinusala

Copy link
Copy Markdown
Owner

I am very sorry but I am currently rewriting the entirety of font handling for the yoga rework (yoga branch, my work is not pushed yet).

I am currently adapting Behemoth's work of splitting borealis in two "platforms", glfw and switch. That means the Switch will have its own font loading code, which is no longer in application.cpp. The loading logic will be different too - everything will be abstracted away from the Switch definitions.

Would you mind redoing what you did on the yoga branch once I push what I have? I will notify you here.

The core of what you did will not be altered, you will just need to put it in the new font code dedicated to the Switch platform.

@CaiMiao

CaiMiao commented Jan 24, 2021 via email

Copy link
Copy Markdown
Author

@natinusala

Copy link
Copy Markdown
Owner

Hey @CaiMiao, I finished working on the new font abstraction layer. It's on the yoga branch, feel free to PR to that branch if you want to.

Now fonts are defined in a fully platform agnostic file, and every platform implementation loads them as they wish. The fallback code is still in application.cpp but the font loading has been moved to glfw_font.cpp and switch_font.cpp.

@CaiMiao

CaiMiao commented Feb 7, 2021 •

Copy link
Copy Markdown
Author

have problem with building demo
massive <command-line>: warning: missing terminating " character
and some errors deny me from building (not only from this file)

<command-line>: error: missing terminating " character
<command-line>: note: in definition of macro 'BRLS_RESOURCES'
C:/src/msys64/home/CaiMiao/borealis/library/lib/core/font.cpp:25:29: note: in expansion of macro 'BRLS_ASSET'
   25 | #define MATERIAL_ICONS_PATH BRLS_ASSET("material/MaterialIcons-Regular.ttf")
      |                             ^~~~~~~~~~
C:/src/msys64/home/CaiMiao/borealis/library/lib/core/font.cpp:51:56: note: in expansion of macro 'MATERIAL_ICONS_PATH'
   51 |     return this->loadFontFromFile(FONT_MATERIAL_ICONS, MATERIAL_ICONS_PATH);
      |                                                        ^~~~~~~~~~~~~~~~~~~
make[1]: *** [/opt/devkitpro/devkitA64/base_rules:14:font.o] 错误 1
make: *** [Makefile:186:all] 错误 2

@natinusala

Copy link
Copy Markdown
Owner

It looks like your BRLS_RESOURCES define is wrong, it's missing a quote

@CaiMiao

CaiMiao commented Feb 7, 2021

Copy link
Copy Markdown
Author

It looks like your BRLS_RESOURCES define is wrong, it's missing a quote

I'm compiling the demo under the yoga branch, and everything is unmodified, so i don't really know where I did wrong.
can only see CXXFLAGS := $(CXXFLAGS) -DYG_ENABLE_EVENTS -fdata-sections -DBRLS_RESOURCES="\"romfs:/\"" in borealis.mk

@natinusala

Copy link
Copy Markdown
Owner

Can you try again with the changes of this commit please?

6ea9f96

@CaiMiao

CaiMiao commented Feb 8, 2021

Copy link
Copy Markdown
Author

Can you try again with the changes of this commit please?

6ea9f96

this works for me, ty. once i tested the implement okay i will PR them again asap.

@CaiMiao

CaiMiao commented Feb 8, 2021

Copy link
Copy Markdown
Author

replaced by #79

@CaiMiao CaiMiao closed this Feb 8, 2021
@CaiMiao
CaiMiao deleted the master branch February 8, 2021 14:52
@CaiMiao
CaiMiao restored the master branch February 8, 2021 15:22
@CaiMiao CaiMiao changed the title Application: Workaround for S./T. Chinese font loading Application: General i18n workaround for shared font loading Feb 8, 2021
@CaiMiao

CaiMiao commented Feb 8, 2021

Copy link
Copy Markdown
Author

well i think it is not bad to merge this to legacy though, in case anyone need to include the shared font workaround without need to update to yoga.

@CaiMiao CaiMiao reopened this Feb 8, 2021
@natinusala

Copy link
Copy Markdown
Owner

The thing is, I don't want to maintain this version anymore. The yoga rework is too hard to rebase since so many things changed 😕

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.

2 participants