Add a font picker that lets the user avoid editing json config files
Summary
Interface "Add a font picker that lets the user avoid editing config files"
Purpose of change
Editing config files is a skill that most people can learn, but editing a config file merely in order to choose a font is a little… much.
Describe the solution
Lets the user select any font file(s) on their system using a file picker. Font fallback order can be adjusted by drag and drop.
Sliders are provided for adjusting font and character cell sizes.
All changes are applied immediately for instant feedback, because making the user restart the game is clumsy and rude.
Testing
Testing was mostly manual. Open the new Font Settings window and try things out. Move the sliders, add and remove font files, etc, etc. Note that you can do this from the main menu as well as in the game.
Additional context
https://db48x.net/cdda/font-picker-demo.mp4
I’m not really sure how to fix the windows build error; it needs some defines added to the list. See the Makefile changes.
could you provide some details about why exactly you need those large inclusions? this is a pretty massive changeset, and an explanation for how it got that way would make it easier to swallow.
i'm also seeing some modifications to some of the imgui files in the third party tree... preferably things like this should be in their own commit with the commit note giving some explanation (are these hand edits? upstream version updates?)
Fair enough :)
In order to be able to pick a file, I needed a file picker. I considered the possibility of using native file pickers at first. On Windows and OSX at least this would be relatively straight forward. But on Linux it’s not so easy. On Linux I would have to pull in a toolkit library like Gtk or Qt (or something even older, like Tk, Motif, Athena…). And then, since I am unable to compile things on Windows or OSX myself I would have to use my Qt/Gtk implementation on those platforms as well. Luckily both of them have cross platform implementations.
But then I decided that if the dialogs aren’t going to be native, perhaps an ImGui–based solution would be best. It might be a big hunk of code, but it’s not nearly the behemoth of Gtk or Qt. And I was able to find a nice one that someone else had written, so I ran with it.
Of course I would rather have a font picker, than a file picker. However, I couldn’t actually find one that someone else had written and writing one again means linking against additional libraries. (We currently open and render a font directly from a file name, but if we wanted the user to just give a font name like “Times New Roman” and then do a search to find the right file then we would want to pull in a different library.)
The modifications to ImGui are to include a function that helps implement drag and drop reordering of elements in an ImGui window. I use it to let the user rearrange the fallback order of the fonts that they have selected. (Any characters not present in the first font in the list are pulled from the next, and so on down the list.) I pulled this implementation off of the ImGui wiki iirc.
I’m not really sure how to fix the windows build error; it needs some defines added to the list. See the Makefile changes.
diff --git a/src/font_picker.cpp b/src/font_picker.cpp
index 97d65f5219..0bfbee8a90 100644
--- a/src/font_picker.cpp
+++ b/src/font_picker.cpp
@@ -16,6 +16,7 @@
#undef IMGUI_DEFINE_MATH_OPERATORS
#include <imgui/imgui_freetype.h>
+#include <ImGuiFileDialog/CustomImGuiFileDialogConfig.h>
#include <ImGuiFileDialog/ImGuiFileDialog.h>
#include <ImGuiFileDialog/CustomFont.h>
#include <ImGuiFileDialog/CustomFont.cpp> // NOLINT(bugprone-suspicious-include)
@@ -61,10 +62,12 @@ static void InitFontFileOpenDialog( font_config &face )
#elif defined(_WIN32)
char *user_dir;
if( ( user_dir = getenv( "windir" ) ) ) {
- places_ptr->AddPlace( "System Fonts", fs::path( user_dir ) / "fonts", false );
+ places_ptr->AddPlace( "System Fonts",
+ ( fs::path( user_dir ) / "fonts" ).generic_u8string(), false );
}
if( ( user_dir = getenv( "LOCALAPPDATA" ) ) ) {
- places_ptr->AddPlace( "User Fonts", fs::path( user_dir ) / "Microsoft" / "Windows" / "Fonts",
+ places_ptr->AddPlace( "User Fonts",
+ ( fs::path( user_dir ) / "Microsoft" / "Windows" / "Fonts" ).generic_u8string(),
false );
}
#elif defined(__ANDROID__)
@@ -248,7 +251,7 @@ void FontPickerWindow::ShowFontOptions( const char *label, const char *window_na
ImGui::SameLine();
ImGui::AlignTextToFramePadding();
fs::path filename = fs::path( face.path ).filename();
- ImGui::TextUnformatted( filename.c_str() );
+ ImGui::TextUnformatted( filename.generic_u8string().c_str() );
ImGui::SameLine();
if( cataimgui::BeginRightAlign( "x" ) ) {
if( ImGui::Button( _( "Details" ) ) ) {
@@ -320,7 +323,7 @@ void FontPickerWindow::ShowFontDetailsWindow( const char *window_name,
}
ImGui::SameLine();
ImGui::AlignTextToFramePadding();
- ImGui::TextUnformatted( filename.c_str() );
+ ImGui::TextUnformatted( filename.generic_u8string().c_str() );
ImGui::SameLine();
if( cataimgui::BeginRightAlign( "right-controls" ) ) {
if( ImGui::BeginCombo( "##hinting",
@@ -355,7 +358,7 @@ void FontPickerWindow::ShowFontDetailsWindow( const char *window_name,
}
}
if( face_to_remove ) {
- typefaces.erase( static_cast<std::vector<font_config>::iterator>( face_to_remove ) );
+ //typefaces.erase( static_cast<std::vector<font_config>::iterator>( face_to_remove ) );
face_to_remove = nullptr;
picker->WantRebuild = true;
}
diff --git a/src/third-party/ImGuiFileDialog/ImGuiFileDialog.h b/src/third-party/ImGuiFileDialog/ImGuiFileDialog.h
index be63842b2b..f63025e047 100644
--- a/src/third-party/ImGuiFileDialog/ImGuiFileDialog.h
+++ b/src/third-party/ImGuiFileDialog/ImGuiFileDialog.h
@@ -196,7 +196,7 @@ struct IGFD_Thumbnail_Info {
#ifdef IMGUI_INCLUDE
#include IMGUI_INCLUDE
#else // IMGUI_INCLUDE
-#include <imgui.h>
+#include <imgui/imgui.h>
#endif // IMGUI_INCLUDE
#include <set>
diff --git a/src/third-party/ImGuiFileDialog/ImWidgets.h b/src/third-party/ImGuiFileDialog/ImWidgets.h
index 14cf3be650..07fccc100d 100644
--- a/src/third-party/ImGuiFileDialog/ImWidgets.h
+++ b/src/third-party/ImGuiFileDialog/ImWidgets.h
@@ -25,7 +25,7 @@
#ifdef IMGUI_INCLUDE
#include IMGUI_INCLUDE
#else // IMGUI_INCLUDE
-#include <imgui.h>
+#include <imgui/imgui.h>
#endif // IMGUI_INCLUDE
#include <cstdint> // types like uint32_t
Cataclysm-lib-vcpkg-static.vcxproj -> C:\Projects\Cataclysm-DDA\objwin\Release\x64\Cataclysm-lib-vcpkg-static\Cataclysm-lib-vcpkg-static-Release-x64.lib
------ Skipped Build: Project: JsonFormatter-lib-vcpkg-static, Configuration: Release x64 ------
Project not selected to build for this solution configuration
------ Build started: Project: Cataclysm-vcpkg-static, Configuration: Release x64 ------
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: __cdecl IGFD::FileStyle::FileStyle(void)" (??0FileStyle@IGFD@@QEAA@XZ) referenced in function "void __cdecl InitFontFileOpenDialog(struct font_config &)" (?InitFontFileOpenDialog@@YAXAEAUfont_config@@@Z)
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: bool __cdecl IGFD::PlacesFeature::GroupStruct::AddPlace(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,bool,class IGFD::FileStyle const &)" (?AddPlace@GroupStruct@PlacesFeature@IGFD@@QEAA_NAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@0_NAEBVFileStyle@3@@Z) referenced in function "void __cdecl InitFontFileOpenDialog(struct font_config &)" (?InitFontFileOpenDialog@@YAXAEAUfont_config@@@Z)
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: bool __cdecl IGFD::PlacesFeature::AddPlacesGroup(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,unsigned __int64 const &,bool,bool)" (?AddPlacesGroup@PlacesFeature@IGFD@@QEAA_NAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEB_K_N2@Z) referenced in function "void __cdecl InitFontFileOpenDialog(struct font_config &)" (?InitFontFileOpenDialog@@YAXAEAUfont_config@@@Z)
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: struct IGFD::PlacesFeature::GroupStruct * __cdecl IGFD::PlacesFeature::GetPlacesGroupPtr(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (?GetPlacesGroupPtr@PlacesFeature@IGFD@@QEAAPEAUGroupStruct@12@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Z) referenced in function "void __cdecl InitFontFileOpenDialog(struct font_config &)" (?InitFontFileOpenDialog@@YAXAEAUfont_config@@@Z)
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: __cdecl IGFD::FileDialog::FileDialog(void)" (??0FileDialog@IGFD@@QEAA@XZ) referenced in function "void __cdecl InitFontFileOpenDialog(struct font_config &)" (?InitFontFileOpenDialog@@YAXAEAUfont_config@@@Z)
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: virtual __cdecl IGFD::FileDialog::~FileDialog(void)" (??1FileDialog@IGFD@@UEAA@XZ) referenced in function "void __cdecl `public: static class FileDialog::Instance * __cdecl IGFD::FileDialog::Instance(class FileDialog::Instance *,bool)'::`2'::`dynamic atexit destructor for '_instance''(void)" (??__F_instance@?1??Instance@FileDialog@IGFD@@SAPEAV12@PEAV12@_N@Z@YAXXZ)
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: bool __cdecl IGFD::FileDialog::Display(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,int,struct ImVec2,struct ImVec2)" (?Display@FileDialog@IGFD@@QEAA_NAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@HUImVec2@@1@Z) referenced in function "bool __cdecl ShowFontFileOpenDialog(void)" (?ShowFontFileOpenDialog@@YA_NXZ)
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: void __cdecl IGFD::FileDialog::Close(void)" (?Close@FileDialog@IGFD@@QEAAXXZ) referenced in function "bool __cdecl ShowFontFileOpenDialog(void)" (?ShowFontFileOpenDialog@@YA_NXZ)
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: bool __cdecl IGFD::FileDialog::IsOk(void)const " (?IsOk@FileDialog@IGFD@@QEBA_NXZ) referenced in function "bool __cdecl ShowFontFileOpenDialog(void)" (?ShowFontFileOpenDialog@@YA_NXZ)
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl IGFD::FileDialog::GetFilePathName(int)" (?GetFilePathName@FileDialog@IGFD@@QEAA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@H@Z) referenced in function "bool __cdecl ShowFontFileOpenDialog(void)" (?ShowFontFileOpenDialog@@YA_NXZ)
Cataclysm-lib-vcpkg-static-Release-x64.lib(font_picker.obj) : error LNK2019: unresolved external symbol "public: void * __cdecl IGFD::FileDialog::GetUserDatas(void)const " (?GetUserDatas@FileDialog@IGFD@@QEBAPEAXXZ) referenced in function "bool __cdecl ShowFontFileOpenDialog(void)" (?ShowFontFileOpenDialog@@YA_NXZ)
C:\Projects\Cataclysm-DDA\msvc-full-features\..\cataclysm-tiles.exe : fatal error LNK1120: 11 unresolved externals
Done building project "Cataclysm-vcpkg-static.vcxproj" -- FAILED.
------ Skipped Build: Project: Cataclysm-test-vcpkg-static, Configuration: Release x64 ------
Project not selected to build for this solution configuration
------ Skipped Build: Project: JsonFormatter-vcpkg-static, Configuration: Release x64 ------
Project not selected to build for this solution configuration
========== Build: 1 succeeded, 1 failed, 2 up-to-date, 3 skipped ==========
I’m not really sure how to fix the windows build error; it needs some defines added to the list. See the Makefile changes.
+#include <ImGuiFileDialog/CustomImGuiFileDialogConfig.h> #include <ImGuiFileDialog/ImGuiFileDialog.h>
Thanks, but ImGuiFileDialog.h already does that if CUSTOM_IMGUIFILEDIALOG_CONFIG is correctly defined. For the Linux build I do that in the Makefile. Can we modify the Windows build to define that as well?
- ImGui::TextUnformatted( filename.c_str() ); + ImGui::TextUnformatted( filename.generic_u8string().c_str() );
Hrm. I know that on Linux the path might not be a UTF8 string at all. Can’t the same happen on Windows, if the file system was created using a specific code page rather than UTF8?
- typefaces.erase( static_cast<std::vector<font_config>::iterator>( face_to_remove ) ); + //typefaces.erase( static_cast<std::vector<font_config>::iterator>( face_to_remove ) );
That won’t do :)
#ifdef IMGUI_INCLUDE #include IMGUI_INCLUDE #else // IMGUI_INCLUDE -#include <imgui.h> +#include <imgui/imgui.h> #endif // IMGUI_INCLUDE
Again, this already has a define that we can set.
Thank you, but I think we can do better.
This is an insanely large patch for a setting that you typically need to change only once then forget about it. Can you propose other uses for a file-picker that would make this worthwhile? I also want to point out that SDL3 has a file-picking API and its potential requirements on our repo are several orders of magnitude smaller.
EDIT: It looks like you can slim this down to a third by dropping dirent (set USE_STD_FILESYSTEM instead), and stb_image/resize (which are only used for thumbnailing).
This is an insanely large patch for a setting that you typically need to change only once then forget about it.
I agree that most people will choose a font and then not need to again. However, I think that many people will try out multiple fonts looking for the right one. They won’t know ahead of time which font they really prefer, so in practice they may use the file picker a dozen times in that one session. Since that saves them a dozen restart cycles I think it really is worth it. Incidentally, this is why the Font Options window is available inside the game and not just at the main menu. I want the user to see the visual impact of their chosen font directly on the sidebar.
Can you propose other uses for a file-picker that would make this worthwhile?
How about allowing the user to import a character portrait to show on their character sheet? Or to save and load container settings (priorities, whitelists, blacklists, etc) as files? Loading whole tilesets; there’s no reason we can’t swap those out live.
I also want to point out that SDL3 has a file-picking API and its potential requirements on our repo are several orders of magnitude smaller.
Oh, right. I considered SDL_ShowOpenFileDialog as well, but this bit of the documentation sounded a bit daunting:
On Linux, dialogs may require XDG Portals, which requires DBus, which requires an event-handling loop. Apps that do not use SDL to handle events should add a call to SDL_PumpEvents in their main loop.
That is just too much real work. And in C++, no less.
EDIT: It looks like you can slim this down to a third by dropping dirent (set
USE_STD_FILESYSTEMinstead), and stb_image/resize (which are only used for thumbnailing).
I didn’t muck with the filesystem stuff because I didn’t think it would matter. Sounds like it matters a lot after all.
I think I did turn off thumbnails though. See where I commented out USE_THUMBNAILS in CustomImGuiFileDialogConfig.h
That is just too much real work. And in C++, no less.
I think you're misreading that. You only need to call SDL_PumpEvents() and we're already doing it in every input loop, etc. The portal stuff is abstracted by SDL - it's just saying that you might need DBus as a runtime dependency and that part is not our problem.
How about allowing the user to import a character portrait to show on their character sheet? Or to save and load container settings (priorities, whitelists, blacklists, etc) as files? Loading whole tilesets; there’s no reason we can’t swap those out live.
These are vaguely valid, but there's no strong reason to support picking files from arbitrary places instead of fixed folders under our root. In fact, I would say that showing a file picker in the middle of gameplay would break immersion.
If we were offering the user the choice of a custom avatar image, why would we make them jump through hoops to put the image into a particular directory? Why not just let them store the image where they want to?
I’m not too worried about immersion here; any sort of options window is immersion–breaking.
We can use file dialogs for color presets loading, custom keybindings, maybe changing user folders.
But judging on how much headache it caused me while I tried to build this on Windows, I'd suggest doing a first version of font selection w/o file dialog in first version (so, just use a list of existing font files in data\font) and we can add file dialogs later.
We can use file dialogs for color presets loading, custom keybindings, maybe changing user folders.
All excellent ideas :)
But judging on how much headache it caused me while I tried to build this on Windows, I'd suggest doing a first version of font selection w/o file dialog in first version (so, just use a list of existing font files in
data\font) and we can add file dialogs later.
I guess it wouldn’t be hard to put ifdefs around the file dialog code so that it isn’t compiled on Windows. Then leave the buttons disabled but with a tooltip saying “Coming Soon” or something, so that the user knows that they are disabled on purpose. People would still be able to edit the config file manually to pick a different font, same as they can today.
i'm going to have to insist on something more firm than zhilkenserg.
before this can go forward, we're going to need something without massive library imports, and a more useful commit history, breaking things into proper commit stages.
if you can give us something smaller and more reviewable, it's more likely we can build on it after, but we really want to avoid large additions that aren't genuinely necessary, and i'm not convinced the features here are, at this time, worth more stuff getting thrown in there. especially at that size.
also i'm going to restate my earlier comment with: changes to existing imgui code needs to be in a distinct commit so that we can track changes in that over time. being able to look at what modifications were done, why, and in discrete commits so they can be shuffled if necessary is critical, otherwise pulling updates from upstream can become much more difficult.
I agree with other people's concerns regarding the size, but this is incredibly cool looking.
I agree with other people's concerns regarding the size, but this is incredibly cool looking.
i knew i forgot to say something. yes: this is definitely cool looking, and i'd definitely like some of the features, it just has to be within better code footprint costs
@ZhilkinSerg, could you test a small change to see if it fixes the Windows build?
diff --git a/msvc-full-features/Cataclysm-common.props b/msvc-full-features/Cataclysm-common.props
index be5dfb0662a8..a8e7f0875a18 100644
--- a/msvc-full-features/Cataclysm-common.props
+++ b/msvc-full-features/Cataclysm-common.props
@@ -66,7 +66,7 @@
<PreprocessorDefinitions>RELEASE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
</ClCompile>
<ClCompile Condition="!$(Configuration.Contains(NoTiles))">
- <PreprocessorDefinitions>TILES;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+ <PreprocessorDefinitions>IMGUI_INCLUDE='"imgui/imgui.h"';IMGUI_INTERNAL_INCLUDE='"imgui/imgui_internal.h"';CUSTOM_IMGUIFILEDIALOG_CONFIG='"CustomImGuiFileDialogConfig.h"';CUSTOM_IMWIDGETS_CONFIG='"CustomImWidgetsConfig.h"';TILES;%(PreprocessorDefinitions)</PreprocessorDefinitions>
</ClCompile>
<ClCompile Condition="$(Configuration.Contains(NoTiles))">
<PreprocessorDefinitions>USE_PDCURSES;PDC_NCMOUSE;PDC_WIDE;IMTUI;%(PreprocessorDefinitions)</PreprocessorDefinitions>
I’m extrapolating from fairly limited information, so I don’t know if it’ll actually work.
Well, that is progress at least.
stupid astyle
Test failure looks unrelated; something about removing a part from a vehicle.
D:\a\Cataclysm-DDA\Cataclysm-DDA\src\cata_imgui.cpp(238,10): fatal error C1083: Cannot open include file: 'font_picker.h': No such file or directory
Uh, what?
https://www.youtube.com/watch?v=qi4w6Leb8RA
Each of these is a hard blocker on its own.
It is in no way justifiable to pull in 20,000+ lines of third party code in order to implement a single very non-core dialogue.
I very actively do NOT want DDA to have the ability to traverse arbitrary parts of the filesystem, it should operate within its designated directories and that's it.
DDA also shouldn't be implementing its own file picker because it will be non-idiomatic for the file system, leave it to the existing file system utilities of the host OS.
Scan a dda-specific fonts directory, enumerate the fonts, then feel free to do fancy hot-loading stuff from there to make it nice. This cuts the complexity of the solution by many orders of magnitude for either negligible loss of functionality or improved functionality, depending on your point of view.
I very actively do NOT want DDA to have the ability to traverse arbitrary parts of the filesystem, it should operate within its designated directories and that's it.
It is perfectly normal for a game to load save files only from one directory, but when it comes to fonts isn’t it perfectly normal to allow the user to pick any font that they have?
I very actively do NOT want DDA to have the ability to traverse arbitrary parts of the filesystem, it should operate within its designated directories and that's it.
It is perfectly normal for a game to load save files only from one directory, but when it comes to fonts isn’t it perfectly normal to allow the user to pick any font that they have?
two weeks ago, when i said you'd need to proceed without the massive imports as a first step, that was not a suggestion.
now when kevin is saying that (among other things), it is still not a suggestion.
it isn't a question of how reasonable you find the functionality to be, this is a non-negotiable item. the why for that issue has already been explained.
As a compromise, why not just whip up a quick UI in imgui where the user can paste all their prospective font file paths
Then instead of loading each file with the file picker, just load them from that list and let the user click back and forth between them with the font switcher
I’ve addressed all of your review comments; thanks again. Let me know if you spot anything else :)
My reading of Kevin's comment is that you need to shift this towards loading all fonts from a directory.
I understood it to mean that we should give the user the choice to add of any font(s) in that directory, letting them also choose the fallback order as already implemented. We can’t just automatically scoop up every font in that directory though. So there’ll have to be some kind of “font picker” that lists the files in that directory and lets the user choose one to add to their font list, even if it is relatively simple compared to what I already implemented.
"present the player with the fonts that are available in the designated directory" is fine. The thing I don't want is "navigate the directory tree to find the fonts".
test failure is unrelated
Need to rebase
Talk to me about this CustomFonts thing. That looks like one or more things that need a license, but there is no license specified. According to the metadata it looks like you're embedding glyphs exerpted from Roboto and FontAwesome WebFont in a custom bundle to supply menu glyphs. Those are both SIL OFL licensed which is fine, but they need this specified. The license doesn't say it needs attribution but for our own use we need things saying where everything came from. Also where are the icons from, did you personally make them or are they sourced from somewhere? If the latter they need a license.
One, I'm a bit skeptical of this particular menu needing different treatment from literally every other menu in the game. Why does this menu need custom glyphs and icons? Two, why is it specified this way? Three, if they're needed, need some metadata specifying the licenses for each element.