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

Use open source Shared Font if no dumped file is available #3881

Merged
merged 8 commits into from Jul 18, 2018

Conversation

Projects
None yet
9 participants
@B3n30
Contributor

B3n30 commented Jun 25, 2018

This will load the shared font generated with https://github.com/B3n30/citra_system_archives if no dumped file was found. The data will be downloaded from this repo during cmake. To enable cmake while there is no internet connection I added the flag 'DENABLE_CUSTOM_SYSTEM_ARCHIVES'.

There are still some letters with minor graphical glitches in the shared font like the m: bildschirmfoto 2018-06-25 um 18 15 25

I didn't remove the legacy method yet, since the shared fonts for CHN/KOR/TWN are still missing and if built without DENABLE_CUSTOM_SYSTEM_ARCHIVES it could still load the legacy fonts


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Jun 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-25T16:21:31Z: 689977b...B3n30:162351798696baa28f26fd0947c695eee47decc7

u64 romfs_offset = 0;
u64 romfs_size = shared_font_file.size();
std::unique_ptr<DelayGenerator> delay_generator =
std::make_unique<RomFSDelayGenerator>();

This comment has been minimized.

@jroweboy

jroweboy Jun 25, 2018

Member

Should this really be using the RomFS delay?

This comment has been minimized.

@B3n30

B3n30 Jun 25, 2018

Contributor

Yes. We still want the emulated time for opening the file, nevertheless if it is an actual file or derived from memory

ResultVal<size_t> IVFCFileVirtual::Write(const u64 offset, const size_t length, const bool flush,
const u8* buffer) {
NGLOG_ERROR(Service_FS, "Attempted to write to IVFC file");
// TODO(Subv): Find error code

This comment has been minimized.

@jroweboy

jroweboy Jun 25, 2018

Member

//TODO(Subv)

is that really fair to him? ;) (I'm fine keeping it as is since its a paste of another todo)

@wwylele

Code side mostly LGTM. Still discussing whether to include the font file into the main repo

@@ -118,4 +119,24 @@ class IVFCDirectory : public DirectoryBackend {
}
};
class IVFCFileVirtual : public FileBackend {

This comment has been minimized.

@wwylele

wwylele Jun 25, 2018

Member

Either add some doc, or renaming it to something more direct one like IVFCFileInMemory

u64 GetSize() const override;
bool SetSize(u64 size) const override;
bool Close() const override {
return false;

This comment has been minimized.

@wwylele

wwylele Jun 25, 2018

Member

why is this returning false (which indicates an error iirc)?

This comment has been minimized.

@wwylele

wwylele Jun 25, 2018

Member

I'll let this pass. Looks like a very old problem in the existing code, and it is not forward to the game

std::make_unique<RomFSDelayGenerator>();
file = std::make_unique<IVFCFileVirtual>(std::move(shared_font_file), romfs_offset,
romfs_size, std::move(delay_generator));
return MakeResult<std::unique_ptr<FileBackend>>(std::move(file));

This comment has been minimized.

@wwylele

wwylele Jun 25, 2018

Member

Please log a warning here.

@wwylele

This comment has been minimized.

Member

wwylele commented Jun 25, 2018

Note: because this currently relies on CI setting the flag, tagging it with canary-merge wouldn't affect windows canary build due to #3706 changing CI platform.

@cluezbot

This comment has been minimized.

cluezbot commented Jun 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-25T17:48:09Z: 689977b...B3n30:263a7777a4f12f8399223a5b74c447f35a75d444

@chris062689

Before merging this. I want to review and move the shared font generation tool into the citra-emu organization.

@chris062689

This comment has been minimized.

Member

chris062689 commented Jun 25, 2018

Just to confirm, are we changing this from downloading from external each time to inside the repo itself?
Regardless I'd like it to be reproducible via the other repository.
Can we include a file hash inside the header? How can we tell if it's reproducible?

@cluezbot

This comment has been minimized.

cluezbot commented Jun 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-25T19:45:23Z: 689977b...B3n30:9a3f6fd3de6a9b0a2d402ae722e4c588b2bc85b9

@B3n30 B3n30 force-pushed the B3n30:VirtualSharedFont branch from 9a3f6fd to bb4f60c Jun 25, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jun 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-25T19:59:54Z: 689977b...B3n30:bb4f60c8eb857d8358692433f4834ce4d0e8d9b8

@@ -42,6 +42,9 @@ target_include_directories(microprofile INTERFACE ./microprofile)
add_library(nihstro-headers INTERFACE)
target_include_directories(nihstro-headers INTERFACE ./nihstro/include)
#Open Source Archives

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 26, 2018

Member

Missing space after #

@@ -19,6 +19,7 @@
#include "core/hle/service/am/am.h"
#include "core/hle/service/fs/archive.h"
#include "core/loader/loader.h"
#include "shared_font.app.romfs.h"

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 26, 2018

Member

Should a longer path be used? It will be hard to find the file only by name.

@cluezbot

This comment has been minimized.

cluezbot commented Jun 26, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-26T12:14:43Z: 689977b...B3n30:f4bd39ec09ae6e4be394e56d4ab61658cd6d3ad7

@B3n30 B3n30 force-pushed the B3n30:VirtualSharedFont branch from f4bd39e to 318b074 Jul 3, 2018

@Krusher97

This comment has been minimized.

Krusher97 commented Jul 9, 2018

citra-qt_2018-07-09_21-11-34
citra-qt_2018-07-09_21-12-20

In Crashmo the A button prompt on the attract mode is not properly rendered when original fonts aren't present.

@cluezbot

This comment has been minimized.

cluezbot commented Jul 9, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-07-09T17:39:06Z: fd5f71b...B3n30:3300b07d27c51ea9a76890c502f8f2516ed2010f

@B3n30

This comment has been minimized.

Contributor

B3n30 commented Jul 9, 2018

Updated the font. There was a stupid bug in the font generator so that symbols got omitted (Thx to @BreadFish64 ). Also the font size was reduced by 2 px so that the m should fit better.

@rcyggdra

This comment has been minimized.

rcyggdra commented Jul 11, 2018

how to used custom font on the lastest Citra?
I had custom font file on path:
.\user\sysdata\shared_font.bin
but it doesn't work.

@BreadFish64

This comment has been minimized.

Contributor

BreadFish64 commented Jul 11, 2018

@rcyggdra that method of loading the font has long been depreciated, and finally removed. The font should be built as a romfs or ncch and placed at .\user\nand\zeros\0004009b\00014002\content\

Any further questions related to this should be taken to our forums or discord channel

@wwylele

LGTM. As an open source font replacement, I don't expect it to be perfect on first try, and it only functions as a mitigation to bad UX of "game stops working due to missing font".

Before merging, I want to know if anyone else has more comments? Also, would it preferable to let CHN/TWN/KOR also use this font for now, so that we won't get mystic "I changed the region and it doesn't work" issue report in the future?

@B3n30

This comment has been minimized.

Contributor

B3n30 commented Jul 18, 2018

Before merging, I want to know if anyone else has more comments? Also, would it preferable to let CHN/TWN/KOR also use this font for now, so that we won't get mystic "I changed the region and it doesn't work" issue report in the future?

I don't think we should CHN/TWN/KOR let use that font but instead implement them with the same method and bring them into citra

@jroweboy jroweboy merged commit 7f1303a into citra-emu:master Jul 18, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment