-
Notifications
You must be signed in to change notification settings - Fork 2.8k
families.csv: Add test VF tags #9897
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
Open
m4rc1e
wants to merge
2
commits into
main
Choose a base branch
from
m4rc1e-patch-37
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the intended loudness at weight 599?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Now I wonder if this could/should be modelled by deltas?
wdth 75 => +6
wdth 100 => +0
wght 600 => +15
wght 800 => +75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I still wonder what wght 599 scores, it would be a bit odd if it jumped directly from 15 to 0)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we let the RBF interpolation do its thing, we'll get a value less than 10. This is what happens in my demo, https://google.github.io/fonts/vf-tag-demo2.html adjust the weight slider and you'll see you'll get values less than the minimum tag, which is also 10.
If we don't like this behaviour, we should instead be defining the coordinate when the score is 0.
We could also treat it like a designspace aka don't allow extrapolation so it will literally be 0 for any value outside the range of 600-800.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this pr in the tagger may make more sense for why I picked the values that I did.
https://google.github.io/fonts/tags.html?categories=%2FExpressive%2FLoud&sort=Score&reverse=false&commit=a6799acc6eb6ad93485a3e544d6d22466b6d1309
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we ruled out RBF and planned to use "normal" VF interpolation, @nyshadhr9 is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intuition is this may be what we want, making triangles of influence, say loud is 0 at 400, high at 900, in traditional VF style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right - we will go with linear/multilinear interpolation.
Marc and I talked offline. Looks like supporting something like a step function - loud between 600 and 800 but not below or above those values - might be helpful too. I think we should be able to support that, but we need to come up with a way to specify this when assigning tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a new demo that uses linear interpolation and doesn't allow extrapolation, https://google.github.io/fonts/vf-tag-demo-linear.html. Imo this feels like the right direction. People adding the tags should also be thinking "when does 0 start for this tag"