Skip to content

[Vello Hybrid + CPU]: Expose render_decoration from Glifo#1592

Merged
taj-p merged 6 commits into
mainfrom
tajp/exposeDecoration
Apr 22, 2026
Merged

[Vello Hybrid + CPU]: Expose render_decoration from Glifo#1592
taj-p merged 6 commits into
mainfrom
tajp/exposeDecoration

Conversation

@taj-p
Copy link
Copy Markdown
Contributor

@taj-p taj-p commented Apr 21, 2026 •
edited
Loading

Mostly Claude Opus 4.6 implemented with some tweaking of tests - simply exposes the render_decoration method of glifo.

@taj-p taj-p requested a review from LaurenzV April 21, 2026 07:32
taj-p
taj-p commented Apr 21, 2026
View reviewed changes
Comment thread sparse_strips/vello_sparse_tests/snapshots/glyphs_decoration_transformed_cached.png
Copy link
Copy Markdown
Contributor Author

@taj-p taj-p Apr 21, 2026

Choose a reason for hiding this comment

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

This looks like a bug. I don't have time left today to debug this. I can investigate tomorrow unless you get to it today

Copy link
Copy Markdown
Contributor

@laurenz-canva laurenz-canva Apr 21, 2026

Choose a reason for hiding this comment

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

I think this should be correct? This is coming from the first glyph run:
image

Since you scale it up by 20, it makes sense that the underline (as well as the gap) become larger as well. Or am I missing something?

Copy link
Copy Markdown
Contributor

@laurenz-canva laurenz-canva Apr 21, 2026

Choose a reason for hiding this comment

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

// Run-level scale absorbs into font_size

For the purpose of glyph caching it should absorb into font size, yes. However, the underline should just be scaled by 20x, otherwise the size of the underline would not change when zooming in!

Copy link
Copy Markdown
Contributor Author

@taj-p taj-p Apr 21, 2026

Choose a reason for hiding this comment

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

Ah yes! Thank you for this. It is correct.

Copy link
Copy Markdown
Contributor Author

@taj-p taj-p Apr 21, 2026

Choose a reason for hiding this comment

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

Made it less ugly in 28685ca

LaurenzV
LaurenzV approved these changes Apr 21, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@LaurenzV LaurenzV left a comment

Choose a reason for hiding this comment

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

I disabled hinting completely and made the tests a bit smaller, let me know if that works for you!

Assuming that the glyphs decoration behavior is intended (which I believe it is), it would maybe be good to adjust the test so it doesn't look that ugly. Otherwise LGTM!

@taj-p taj-p enabled auto-merge April 21, 2026 23:21
@taj-p taj-p added this pull request to the merge queue Apr 21, 2026
Merged via the queue into main with commit 8183484 Apr 22, 2026
17 checks passed
@taj-p taj-p deleted the tajp/exposeDecoration branch April 22, 2026 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laurenz-canva laurenz-canva laurenz-canva left review comments

@LaurenzV LaurenzV LaurenzV approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@taj-p @LaurenzV @laurenz-canva