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

🤖🪵 Fix emoji placement by calculating full bounds #30

Merged
merged 2 commits into from
Nov 18, 2022

Conversation

Jasper-Bekkers
Copy link
Contributor

@Jasper-Bekkers Jasper-Bekkers commented Nov 16, 2022

Addresses incorrect emoji rendering as reported in #26

Previously the placement of the very first colored outline was used to calculate image dimensions, however this is incorrect. One should use the full bounds of all outlines for calculating this size.

Notice that this also needs a tiny PR on zeno dfrg/zeno#4

Attached are correctly rendered 🤖 and 🪵 emoji's as per the issue file.

stuff
stuff

@Jasper-Bekkers
Copy link
Contributor Author

Examples of this rendered in cosmic-text;

image

@dfrg
Copy link
Owner

dfrg commented Nov 17, 2022

Thanks! This looks good but I did notice that Outline already has a bounds method (https://docs.rs/swash/latest/swash/scale/outline/struct.Outline.html#method.bounds) that computes the full bounding box. I assume this will return the same result as your code without requiring the change to zeno though I’m happy to accept that one as well.

@Jasper-Bekkers
Copy link
Contributor Author

This is what I'm using, however, Bounds didn't have a way to grow yet; which is what I PR'd to Zeno.

@dfrg
Copy link
Owner

dfrg commented Nov 17, 2022

Right, but Outline::bounds should already return the union of all layer bboxes so there’s no need to calculate it manually.

@Jasper-Bekkers
Copy link
Contributor Author

Right, but Outline::bounds should already return the union of all layer bboxes so there’s no need to calculate it manually.

Thanks for the heads up! I've update the code to just call this instead and that seems to work fine as well (re-ran my tests for it).

@dfrg
Copy link
Owner

dfrg commented Nov 18, 2022

Thanks for fixing this! I’ll bump the version and publish a new release to crates later today.

@Jasper-Bekkers
Copy link
Contributor Author

Thanks! A release would be quite welcome 👍

@dfrg
Copy link
Owner

dfrg commented Nov 18, 2022

Released 0.1.5!

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.

2 participants