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

Font and style #173

Merged
merged 11 commits into from
Aug 30, 2024
Merged

Font and style #173

merged 11 commits into from
Aug 30, 2024

Conversation

AKMaily
Copy link
Collaborator

@AKMaily AKMaily commented Aug 28, 2024

Added a new Font and changed the corresponding submodule ImGuiInstance to the version where a different font is possible.

Changed the fixed fontsize in the config to match the new Font when the application starts the first time.

Added a different AI Logo for the OmnAIScope and changed its size.

Changed the colors in the application to match the logo and the overall design more.

The expected output looks like this:

image

The initial fontsize will be to large when the application first starts because it currently downloads the config from the master branch. This will be fixed when this PR is merged with the master.

@AKMaily AKMaily added the enhancement New feature or request label Aug 28, 2024
@AKMaily AKMaily requested a review from R-Abbasi August 28, 2024 13:52
Copy link
Collaborator

@R-Abbasi R-Abbasi left a comment

Choose a reason for hiding this comment

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

It seems fine but the executable doesn't run on my machine.
Did you test that too, please?

@AKMaily
Copy link
Collaborator Author

AKMaily commented Aug 29, 2024

Yes i did. Did you build it yourself or did you use one from the CI build ?

@R-Abbasi
Copy link
Collaborator

Used the executable from CI.

@R-Abbasi
Copy link
Collaborator

You may also use inline constexpr unsigned char AutoInternLogo_png[].

@R-Abbasi
Copy link
Collaborator

The path "config/config.json"; should be correct since it looks for the folder where the executable exists.

@R-Abbasi
Copy link
Collaborator

Have you tested something like this, please?

if (io.Fonts->AddFontFromFileTTF("fonts/Inter_24pt-Medium.ttf", 24.0f) != nullptr) {
              font2 = io.Fonts->AddFontFromFileTTF("fonts/Inter_24pt-Medium.ttf", 24.0f);
       }

The fonts folder is put in the same directory as the config one apparently.

@AKMaily
Copy link
Collaborator Author

AKMaily commented Aug 29, 2024

The path "config/config.json"; should be correct since it looks for the folder where the executable exists.

Yes it was i changed it back to the original version

@AKMaily
Copy link
Collaborator Author

AKMaily commented Aug 29, 2024

Have you tested something like this, please?

if (io.Fonts->AddFontFromFileTTF("fonts/Inter_24pt-Medium.ttf", 24.0f) != nullptr) {
              font2 = io.Fonts->AddFontFromFileTTF("fonts/Inter_24pt-Medium.ttf", 24.0f);
       }

The fonts folder is put in the same directory as the config one apparently.

Yes i tested that, the problem is that the fonts are not downloaded when the .exe starts, so the font can not be found. I am currently working on including the fonts in a binary file and using AddFontFromMemoryTTF.

@R-Abbasi
Copy link
Collaborator

I've not used custom fonts before but if you download the font and put it in the related folder, just like the json files, and provide with AddFontFromFileTTF the absolute path to it, it may work.

@AKMaily
Copy link
Collaborator Author

AKMaily commented Aug 29, 2024

Included it as a binary file. It should work now.

@R-Abbasi
Copy link
Collaborator

R-Abbasi commented Aug 30, 2024

These're a few remaining notes at once:

  • Font and style #173 (comment)
  • you can put your font array into a header pretty much the same way we used imagesHeader, and remove Inter_24pt-Medium.cpp :
// fontHeader.hpp
#ifndef FONT_HEADER_HPP
#define FONT_HEADER_HPP

inline unsigned char Inter_24pt-Medium[] = {
// ..
 
inline constexpr size_t Inter_24pt-Medium_len =
sizeof(Inter_24pt-Medium) / sizeof(Inter_24pt-Medium[0]);

 #endif
  • CMakeLists file needn't be changed, I suppose.

As well as, 20.f font pixels' size would suffice, I assume.

@AKMaily
Copy link
Collaborator Author

AKMaily commented Aug 30, 2024

Thank you for the review. I will change the datatype of the font data.

I tried using the method you suggested for the Font_Header before, the same way we use it for the images but it did not work.

The Error i get is :


(.text+0x0): multiple definition of `Inter_24pt_Medium_ttf'; CMakeFiles/OmniView.dir/src/main.cpp.o (symbol from plugin):(.text+0x0): first defined here
/usr/bin/ld: CMakeFiles/OmniView.dir/src/style.cpp.o (symbol from plugin): in function `stbi_failure_reason':
(.text+0x0): multiple definition of `Inter_24pt_Medium_ttf_len'; CMakeFiles/OmniView.dir/src/main.cpp.o (symbol from plugin):(.text+0x0): first defined here
/usr/bin/ld: CMakeFiles/OmniView.dir/src/style.cpp.o (symbol from plugin): in function `stbi_failure_reason':
(.text+0x0): multiple definition of `Inter_24pt_Medium_ttf'; CMakeFiles/OmniView.dir/src/main.cpp.o (symbol from plugin):(.text+0x0): first defined here

I defined the header with ifndef and define and included it.

@R-Abbasi
Copy link
Collaborator

Presumably you've not used inline.
Also you need to include fontHeader.hpp only in the ImGuiInstance.hpp. And it should work.

@AKMaily
Copy link
Collaborator Author

AKMaily commented Aug 30, 2024

Thank you! Done that, it works now.

@R-Abbasi
Copy link
Collaborator

Great!
You forgot one little thing. Use inline constexpr unsigned int Inter_24pt-Medium_len = sizeof(Inter_24pt_Medium_ttf) / sizeof(Inter_24pt_Medium_ttf[0]); instead of inline unsigned int Inter_24pt_Medium_ttf_len = 342936; at the end of your header, please.

@AKMaily
Copy link
Collaborator Author

AKMaily commented Aug 30, 2024

Could you explain why that is a better solution?

@R-Abbasi
Copy link
Collaborator

R-Abbasi commented Aug 30, 2024

  • constexpr: to explicitly tell the compiler to do things at compile-time rather than run-time
  • If you use 342936 as the size and change it, E.g., drop a digit of that mistakenly (34293), the code will still work but a crash or unexpected result may happen. With sizeof(Inter_24pt_Medium_ttf) / sizeof(Inter_24pt_Medium_ttf[0]) that risk decreases because a change to it results in a compile-time error which can be caught and fixed much easier.

@AKMaily
Copy link
Collaborator Author

AKMaily commented Aug 30, 2024

Done.

@R-Abbasi
Copy link
Collaborator

Here's my header:

// fontHeader.hpp
#ifndef FONT_HEADER_HPP
#define FONT_HEADER_HPP

inline unsigned char Inter_Medium[] = {
 // ...
 };

 inline constexpr unsigned Inter_Medium_len =
 sizeof(Inter_Medium) / sizeof(Inter_Medium[0]);
 #endif

@AKMaily AKMaily merged commit bf3380f into Dev_v1.0.0 Aug 30, 2024
3 checks passed
@AKMaily AKMaily deleted the FontAndStyle branch August 30, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants