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

refactor: add default_font, change font api #66

Merged
merged 3 commits into from
Jul 20, 2024
Merged

Conversation

simbleau
Copy link
Member

@simbleau simbleau commented Jul 20, 2024

Added

  • There is now a default_font feature that uses the same FiraMono-subset.ttf font used in the bevy/default_font feature.

Changed

  • The font API has changed significantly. Please visit examples/text for further usage. This is to prepare for additional text features such as linebreak behavior, bounded text, and text justification.
    • VelloText has been renamed to VelloTextSection.
    • VelloText.content has been renamed to VelloText.value.
    • There is now a VelloTextStyle struct and it is a required field of VelloText.
    • VelloFont has been removed from VelloTextBundle and moved into VelloTextStyle.

Base automatically changed from refactor-naming to main July 20, 2024 18:55
@simbleau simbleau enabled auto-merge July 20, 2024 19:12
@simbleau simbleau added this pull request to the merge queue Jul 20, 2024
Merged via the queue into main with commit 5882f36 Jul 20, 2024
6 checks passed
@simbleau simbleau deleted the text-refactoring branch July 20, 2024 19:17
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

We need to make sure we're handling the licensing properly.

Can we ask the Bevy project to make a crate containing just the font file, so that the usages can be properly shared?

Copy link
Member

Choose a reason for hiding this comment

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

What's the license for this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

OFL 1.1

Copy link
Member

Choose a reason for hiding this comment

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

If you are using both this crate with default_font and bevy/default_font, does your binary include both?

I do see that the file is relatively small, so it's probably fine.

Copy link
Member Author

@simbleau simbleau Jul 22, 2024

Choose a reason for hiding this comment

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

We aren't currently using bevy/default_font, not until bevyengine/bevy#14406 is merged at least.

@simbleau
Copy link
Member Author

We're good - see

bevyengine/bevy#14406

We recently submitted a PR to expose the font data under current licensing. Which is approved, waiting merge.

So we will switch to that as soon as we can.

@DJMcNab
Copy link
Member

DJMcNab commented Jul 22, 2024

I don't agree with @alice-i-cecile's reading of the OFL in bevyengine/bevy#8445. According to their FAQ:

The only situation in which an OFL font can be distributed without the text of the OFL (either in a separate file or in font metadata), is when a font is embedded in a document or bundled within a program. In the case of metadata included within a font, it is legally sufficient to include only a link to the text of the OFL on https://openfontlicense.org/, but we strongly recommend against this. Most modern font formats include metadata fields that will accept the full OFL text, and full inclusion increases the likelihood that users will understand and properly apply the license.

I can't find the license text within the font file (at least using fc-query), and this use case (hosted on GitHub) is not "bundled within a program" (even though it will be once the program is built).
Even if Bevy has decided it's OK for them, if we're distributing this file, I strongly want us to make it clear that it's under the OFL.

So we should either just remove this file and only use the capability from that PR, or add the OFL.

@simbleau
Copy link
Member Author

simbleau commented Jul 22, 2024

I don't agree with @alice-i-cecile's reading of the OFL in bevyengine/bevy#8445. According to their FAQ:

The only situation in which an OFL font can be distributed without the text of the OFL (either in a separate file or in font metadata), is when a font is embedded in a document or bundled within a program. In the case of metadata included within a font, it is legally sufficient to include only a link to the text of the OFL on https://openfontlicense.org/, but we strongly recommend against this. Most modern font formats include metadata fields that will accept the full OFL text, and full inclusion increases the likelihood that users will understand and properly apply the license.

I can't find the license text within the font file (at least using fc-query), and this use case (hosted on GitHub) is not "bundled within a program" (even though it will be once the program is built). Even if Bevy has decided it's OK for them, if we're distributing this file, I strongly want us to make it clear that it's under the OFL.

So we should either just remove this file and only use the capability from that PR, or add the OFL.

I always planned on switching with the data from the PR, however I am not opposed to adding the OFL sooner until we can do that. I'd gladly accept a PR to add the OFL.

github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this pull request Jul 22, 2024
# Objective

- Enables use cases where third-party crates would want to use the
default font as well [see linebender's
use](linebender/bevy_vello#66)

## Solution

- Uses `include_bytes` macro and make it `pub`

---------

Co-authored-by: Spencer C. Imbleau <[email protected]>
Co-authored-by: BD103 <[email protected]>
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.

3 participants