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

Add preload-font for CLJS version and fix available-fonts function #203

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Norgat
Copy link
Member

@Norgat Norgat commented Feb 21, 2017

Start point: https://groups.google.com/forum/#!topic/clj-processing/9S47cbGQ3bY

Reason for adding new function: Processing.js load font through css font-family and parse @pjs tag in comments.

Example of usage: https://github.com/Norgat/quil/blob/master/test/cljs/snippets/manual.cljs#L157

Processing.js docs:
http://processingjs.org/reference/pjs%20directive/
http://processingjs.org/reference/font/

Externs for Processing.js updated: quil/processing-js@f6c8321

P.S. call @cassiel

@@ -16,7 +16,7 @@
[quil/processing-svg "3.2.2"]
[quil/jogl-all-fat "2.3.2"]
[quil/gluegen-rt-fat "2.3.2"]
[quil/processing-js "1.6.3.0"]
[quil/processing-js "1.6.4.1"]
Copy link
Member

Choose a reason for hiding this comment

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

Why are you updating processing-js version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previous version of quil/processing-js is 1.6.4.0.

I add new section for PFont and change version number to 1.6.4.1 (because we have a new saved words in externs). Commit: quil/processing-js@f6c8321

I don't know we 1.6.3.0 version used in current Quil project file.

Copy link
Member

Choose a reason for hiding this comment

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

You should pull master branch. Currently 1.6.4.0 is used: https://github.com/quil/quil/blob/master/project.clj#L19

Also I need to push 1.6.4.1 to clojars then.

Sample:
(q/preload-font \"ComicSans.ttf\")
(q/text-font
(q/create-font \"ComicSans.ttf\" 14))
Copy link
Member

Choose a reason for hiding this comment

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

Can we do font preloading inside create-font so that user won't need to do it by themselves?

Copy link
Member Author

Choose a reason for hiding this comment

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

This increase the gap between Processing.js docs and Quil.
Do you think that is good idea? Because we hide font loading process from user in this case.

So, what happened if user call create-font inside :draw handler with slow fonts cdn?
I think he will see strange artifacts.

P.S. Yes, fonts are cached inside Processing.js and we don't need to reload it.

Copy link
Member

Choose a reason for hiding this comment

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

There is no preload-font in processing.js anyway, so it's not increasing gap. Quil is not a low level library so it's better if we hide these implementation details from users. If you user calls create-font from inside :draw then only the first call to preloading will have effect and all others will be cached so there won't be performance issues.

But what happens if user tries to preload built-in font? Is it possible to know if user is using built-in font or font from a file?

[font-url]
(.add
(aget js/Processing "prototype" "PFont" "preloading")
font-url)))
Copy link
Member

Choose a reason for hiding this comment

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

This should have the same indentation as aget:

(.add 
  (aget ...
  font-url))

Otherwise it looks like font-url is argument for aget

@@ -16,7 +16,7 @@
[quil/processing-svg "3.2.2"]
[quil/jogl-all-fat "2.3.2"]
[quil/gluegen-rt-fat "2.3.2"]
[quil/processing-js "1.6.3.0"]
[quil/processing-js "1.6.4.1"]
Copy link
Member

Choose a reason for hiding this comment

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

You should pull master branch. Currently 1.6.4.0 is used: https://github.com/quil/quil/blob/master/project.clj#L19

Also I need to push 1.6.4.1 to clojars then.

Sample:
(q/preload-font \"ComicSans.ttf\")
(q/text-font
(q/create-font \"ComicSans.ttf\" 14))
Copy link
Member

Choose a reason for hiding this comment

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

There is no preload-font in processing.js anyway, so it's not increasing gap. Quil is not a low level library so it's better if we hide these implementation details from users. If you user calls create-font from inside :draw then only the first call to preloading will have effect and all others will be cached so there won't be performance issues.

But what happens if user tries to preload built-in font? Is it possible to know if user is using built-in font or font from a file?

@nbeloglazov
Copy link
Member

Maksim you're not waiting on me, right? We still need to resolve the issue "new preload-font" or "update create-font".

@Norgat
Copy link
Member Author

Norgat commented Mar 2, 2017

@nbeloglazov I await your decision. Do you accept or not its current changes?
I expressed my opinion in my code. I prefer preload-font way.

If you have different opinion - write it. I described all available data and possible ways to resolve this problem.

@nbeloglazov
Copy link
Member

Having single create-font is less confusing than both create-font and preload-font. It's not clear how "creating" is different from "preloading". It's better to hide low-level implementation details from users, especially given that quil is fairly high-level library.

I can't think of any performance issues if create-font is called inside :draw: only the first call loads the font while all the rest do nothing. If user still wants to optimize it - they can move create-font call to :setup.

@Norgat
Copy link
Member Author

Norgat commented Mar 2, 2017 via email

@marco-m
Copy link
Contributor

marco-m commented May 2, 2018

Hello, any news ?

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.

None yet

3 participants