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

Remove restriction on rendering 4-byte characters on pygame.font part 2 of 2 #2746

Merged
merged 3 commits into from Oct 24, 2021

Conversation

illume
Copy link
Member

@illume illume commented Oct 5, 2021

continued on from #2627

Just a little bug fix for those changes to pass CI (by disabling the old test).

@illume
Copy link
Member Author

illume commented Oct 5, 2021

The SDL1 build fails to compile src_c/surface_fill.c but doesn't say why. Doesn't seem related to this PR. It doesn't give a reason. https://github.com/pygame/pygame/pull/2746/checks?check_run_id=3800737378#step:6:407

SDL_TTF_VERSION_ATLEAST wasn't defined until recently... which makes using it problematic with old code. No idea why it crashed the compiler on src_c/surface_fill.c... but seems to work now.

@illume illume force-pushed the AvaxarXapaxa-patch-2 branch 3 times, most recently from 8592edd to b323ed7 Compare October 5, 2021 10:20
Copy link
Contributor

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

This PR is very useful indeed!
Hehe, Avaxar had asked me on discord to continue on his PR and I got some work done locally on this a while back, but forgot to finish up and push. Anyways, no probs! We can go ahead with this PR, and the #define of SDL_TTF_VERSION_ATLEAST when it is missing is nice!

Before this PR is merged, it would need a doc update to reflect this change, and also I was thinking, a way to check at runtime if pygame.font module supports ucs4 range would be something useful to people

@illume illume added this to the 2.0.2 milestone Oct 8, 2021
@illume
Copy link
Member Author

illume commented Oct 9, 2021

How about I add the doc update, and for now detecting usc4 can be done with a pygame version check of >= 2.0.2?

@ankith26
Copy link
Contributor

ankith26 commented Oct 9, 2021

Well uhm... There is a chance that someone has pygame 2.0.2 but not SDL_ttf v2.0.15. So users checking for pygame version is a not-so-good idea. Maybe this feature can wait for 2.0.3? but IDK

I would certainly prefer to have like an attribute check, something like

if hasattr(pygame.font, "ucs4"):
    do_ucs4_stuff()
else:
    do_non_ucs4_stuff()

Also this way, the checking code remains compatible with older pygame versions, because introducing a new check function would ofc not work on older pygame versions

@ankith26 ankith26 modified the milestones: 2.0.2, 2.0.3 Oct 11, 2021
@ankith26
Copy link
Contributor

Mind if I push to this branch and finish this PR? Or @illume do you want to continue on this?

@robertpfeiffer
Copy link
Contributor

robertpfeiffer commented Oct 18, 2021

Does this mean we can render emoji now? Are they colourful or monochrome? Do they use the colour passed in by the caller?

@illume
Copy link
Member Author

illume commented Oct 23, 2021

Yeah, the ucs4 check would be nice.

IMHO: I can at least document this now, and it could be merged. The ucs4 can happen at any time later.

@ankith26
Copy link
Contributor

ankith26 commented Oct 23, 2021

hmmm, aight! Sounds like a plan for now. This feature would need some more testing as well (also maybe more unit tests for this), but that too can happen after this PR

Add handling for SDL below version 2.0.15
Comment out old test for UCS-4 error with old SDL_ttf

Co-authored-by: René Dudfield <renesd@gmail.com>
@illume illume force-pushed the AvaxarXapaxa-patch-2 branch 2 times, most recently from 5dd3e2b to 8048408 Compare October 23, 2021 16:44
Copy link
Contributor

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

This PR looks good to go! 🎉 🥳
Left a minor review though

docs/reST/ref/font.rst Outdated Show resolved Hide resolved
@illume illume force-pushed the AvaxarXapaxa-patch-2 branch 2 times, most recently from 13bcf9c to 7527118 Compare October 23, 2021 16:57
@illume illume merged commit 880f6c0 into main Oct 24, 2021
@illume illume deleted the AvaxarXapaxa-patch-2 branch October 24, 2021 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants