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

Allow overriding fonts via media files #15606

Merged
merged 24 commits into from
Jan 19, 2025
Merged

Conversation

appgurueu
Copy link
Contributor

@appgurueu appgurueu commented Dec 30, 2024

Implements #10416 in its most basic form, allowing you to override the fonts that are currently used via media files. Does not include font escape sequences.

To do

This PR is ready for review.

  • Decide on the font file format(s) we want to support:
    • Opted for TTF (for convenience) and WOFF2 (compression, tends to be 2x-3x smaller) for now.
    • Feel free to propose more (or less).
  • Basic security considerations:
    • Probably not terrible since that's a FreeType problem and big browsers use FreeType.
    • Looking at the CVEs for FreeType, not much (critical) vulnerabilities seem to have been found since 2015: https://www.cvedetails.com/vendor/4535/Freetype.html
    • Generally I think this is not very different in principle to relying on the safety of other dependencies, e.g. libpng, libjpeg etc. which are used to load images, though indeed font file formats may tend to be a bit more problematic.
    • If someone has a good suggestion for a font format that is demonstrably less problematic yet not comparatively obscure or inefficient, please share it.

How to test

  • Enable this mod:
    fonts.zip

  • Join game.

  • Observe different fonts. Use /show_fonts if you want to see all 8 different fonts.

  • Leave. Observe normal fonts.

Screenshot from 2025-01-10 02-27-17

If you're feeling funny you can also test with dynamic media.

@appgurueu appgurueu added @ Script API Feature ✨ PRs that add or enhance a feature @ Client rendering labels Dec 30, 2024
@appgurueu appgurueu marked this pull request as draft December 30, 2024 19:59
@appgurueu appgurueu changed the title Allow overriding fonts via media files (PoC) Allow overriding fonts via media files Jan 10, 2025
@appgurueu appgurueu marked this pull request as ready for review January 10, 2025 02:45
@appgurueu appgurueu added this to the 5.11.0 milestone Jan 10, 2025
src/client/fontengine.cpp Outdated Show resolved Hide resolved
src/client/fontengine.cpp Outdated Show resolved Hide resolved
src/client/fontengine.cpp Outdated Show resolved Hide resolved
src/client/fontengine.cpp Show resolved Hide resolved
src/irrlicht_changes/CGUITTFont.cpp Outdated Show resolved Hide resolved
src/irrlicht_changes/CGUITTFont.cpp Outdated Show resolved Hide resolved
src/irrlicht_changes/CGUITTFont.cpp Outdated Show resolved Hide resolved
src/irrlicht_changes/CGUITTFont.cpp Show resolved Hide resolved
src/irrlicht_changes/CGUITTFont.h Outdated Show resolved Hide resolved
doc/lua_api.md Show resolved Hide resolved
@Zughy
Copy link
Contributor

Zughy commented Jan 13, 2025

Shouldn't we feature a fonts folder? In general, current PR's DOCS don't state where font files should be put

@appgurueu
Copy link
Contributor Author

Shouldn't we feature a fonts folder? In general, current PR's DOCS don't state where font files should be put

This PR supports a fonts folder for mods, same as other media folders such as "models". It is technically "documented" in a heading: https://github.com/minetest/minetest/pull/15606/files#diff-c60ce2a4d38acfedec97ac1b19e9e207aa60f223f1b7fdd1048c00bdf1f8d5d9R268.
I've added a very minor commit which hopefully makes this clearer: e4eb8c1.

Copy link
Collaborator

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

I have no code-wise complaints here.

@appgurueu appgurueu merged commit 547e147 into luanti-org:master Jan 19, 2025
16 checks passed
@appgurueu appgurueu deleted the font-media branch January 19, 2025 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants