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

Type 2.0 #7356

Open
wants to merge 88 commits into
base: dev-2.0
Choose a base branch
from
Open

Type 2.0 #7356

wants to merge 88 commits into from

Conversation

dhowe
Copy link
Contributor

@dhowe dhowe commented Nov 3, 2024

Initial PR for typography module 2.0

There are 7 new functions added to the p5 prototype that should be discussed:

  • textBounds: tight bounds, now supported for all fonts, not just those with paths as in v1
  • fontBounds: gives the (loose) bounds for the font, regardless of characters used
  • fontWidth: gives the (loose) width of a string of given length in the font, regardless of the characters
  • textProperty: allows set/get for one of about 20 font-related properties
  • textProperties: allows batch set/get for one or more of about 20 font-related properties
  • textToPoints: a quick/dirty version using pixel-sampling that doesn't require font paths
  • textDirection: exposing this directly for wider access

Additionally textAscent and textDescent now operate differently depending on whether a specific string is passed in. If so, they return the metrics for that specific string, if not, then for the font itself (the max ascent and descent)

Issues/Questions:

  • It seems that there should be a direct get/set option for CanvasRenderingContext2D.font, as it would be useful/common to grab a long css font-string and apply it directly in p5. This cannot be done using textFont without major changes, so I would argue for a new function to handle this.
  • Similarly it would be useful to be able to accept a complete font-face declaration: @font-face { font-family: 'Diwani'; font-weight: 400; font-stretch: normal; src: url('https://fonts.gstatic.com/s/notonaskharabic/v17/RrQ5bpV-9Dd1b1OAGA6M9PkyDuVBePeKNaxcsss0Y7bwvc5Urqc3.ttf') format('truetype'); font-style: normal; }
  • Better handling of google fonts: currently the workflow for integrating such fonts is a bit awkward, either involving finding a font, getting the correct link, opening the linked file and manually copy/pasting a path to a woff or ttf, which then ignores the supplied font-descriptors, or downloading a zip with many different files. Ideally one would simply copy/paste the embed path into loadFont... but there are many complications here, including the fact that many individual fonts would be loaded over the network.
  • Would be great to have some better examples in arabic and other rtl languages for testing
  • Should type functions accept and coerce non-string arguments, eg. text(1000, 20, 20) or allow FES to handle this, and what about for the non-FES case? (see #6298).
  • Similarly, should functions that accept a constant (textWrap() for example), check that it is a valid constant (one of those defined), or should we let FES do that, or neither ?

@limzykenneth
Copy link
Member

The last one failing test is a false negative I think. We'll probably need to revert the screenshots.

@davepagurek
Copy link
Contributor

I'll need to check to see what's up with that one failing test case, it looks like lines are rendering behind the fills. I'm not 100% sure just yet what's going on there because I've seen it on a few people's computers before but not on CI. In any case, it seems completely unrelated to your code, so feel free to merge if it's just that one test failing (and in general just ping me if you see weird visual test failures like that, it's probably not your doing!)

});

test('loadFont.then', () => {
myp5.loadFont(fontFile).then(pFont => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to make this test function async and then await this to make sure the assertions happen within this test case

test/unit/type/loading.js Show resolved Hide resolved
myp5.loadFont('/test/unit/assets/Inconsolata-Bold.ttf', function(font) {
const webgl2 = myp5.createGraphics(100, 20, myp5.WEBGL);
const webgl1 = myp5.createGraphics(100, 20, myp5.WEBGL);
webgl1.setAttributes({ version: 1 });
webgl1.setAttributes({ version: 1 }); // no longer exists ?
Copy link
Contributor

Choose a reason for hiding this comment

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

@limzykenneth we were getting an error on this saying that setAttributes doesn't exist on the graphic. I think this test was just not getting run before (the path to the font was wrong so the success callback in loadFont wasn't running, and because this test neither returns a promise nor uses a done callback, the failure wasn't getting reliably detected. this still needs to be updated I think.)

We're marking this as .todo for now but this might be something we need to add back to p5.Graphics

@davepagurek
Copy link
Contributor

  • It seems that there should be a direct get/set option for CanvasRenderingContext2D.font, as one would often like to grab a long css font-string and apply it directly in p5. This cannot be done using textFont without major changes, so I would argue for a new function to handle this.

Did you have some thoughts about what this API would look like?

@dhowe
Copy link
Contributor Author

dhowe commented Nov 10, 2024

It seems that there should be a direct get/set option for CanvasRenderingContext2D.font, as one would often like to grab a long css font-string and apply it directly in p5. This cannot be done using textFont without major changes, so I would argue for a new function to handle this.

Did you have some thoughts about what this API would look like?

It could be a single function forceTextFont(str) or an option for textFont(str, {direct: true}). I haven't come up with anything good as yet in terms of names. There is a similar case with loadFont() where one could provide an entire font-face declaration (as a string or js object)

@dhowe dhowe mentioned this pull request Nov 10, 2024
14 tasks
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.

4 participants