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

Implementing Typography for Skia #376

Merged
merged 13 commits into from
Aug 13, 2022
Merged

Implementing Typography for Skia #376

merged 13 commits into from
Aug 13, 2022

Conversation

tushar5526
Copy link
Member

@tushar5526 tushar5526 commented Aug 8, 2022

Closes #373
Related to #371

@tushar5526 tushar5526 marked this pull request as draft August 8, 2022 13:54
- Implemented text_font and text
@tushar5526 tushar5526 marked this pull request as ready for review August 8, 2022 14:38
@tushar5526 tushar5526 requested a review from ziyaointl August 8, 2022 14:39
@tushar5526
Copy link
Member Author

Hey @ziyaointl

I have called the preload right there in the userspace.py because it seemed the easy way instead of putting it in the base.py and calling it somewhere in the sketch class.

Also preload does not affect frame_count or any other sketch settings in any way so I called it in the userspace.py as of now.

Let me know your thoughts, and you can review this PR. It is good to go.

I will do multiple PRs this time and use issue #371 to keep track of the implementing typography status, instead of a single PR with many changes.

Copy link
Member

@ziyaointl ziyaointl left a comment

Choose a reason for hiding this comment

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

Great work! I really appreciate the effort that you took to split up large PRs into small ones. This makes reviewing so much easier :D

One nit: I think that adding preload is conceptually different enough from the other font functions that this PR could be split up further, but this is just for future reference - no need to split up this particular PR now.

@@ -109,7 +101,7 @@ def text_font(font, size=10):
:type font: PIL.ImageFont.ImageFont

"""
p5.renderer.font_family = font
p5.renderer.text_font(font, size)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing in a default size of 10, we should probably use the current font size.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, fixed it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Looks like the size parameter is being ignored by vispy's text_font, could you also do a quick fix for that as well?

@ziyaointl
Copy link
Member

And yes, I think putting preload in userspace.py is very reasonable :)

@tushar5526
Copy link
Member Author

I have updated the PR and added a test for typography

@ziyaointl
Copy link
Member

Kudos for adding the tests!

- Added text_style for skia
- Skia can also make fonts from FontFamily, addded support for that in p5py
@ziyaointl ziyaointl merged commit 5e0ae59 into master Aug 13, 2022
@ziyaointl ziyaointl deleted the typography branch August 13, 2022 19:20
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.

Implement preload
2 participants