Add cosmic text example #739

AustinMReppert · 2024-11-19T23:08:11Z

Add an example for cosmic text.

Related:
306

DJMcNab · 2024-11-20T10:25:43Z

Thanks, it's really good to see this.

Would you consider making this a test scene (i.e. in the scenes crate?). Possibly behind a feature gate.
The global context of fonts can be part of the shared SceneParams .

I think that would be useful because this new example has a lot of interleaving of logic between cosmic text and Vello and winit.
Additionally, it could allow us to use cosmic text in our tests.

If not, I think we can still land this nearly as-is, although it being another usage of winit is likely to cause us pain in future when we need to perform updates.

AustinMReppert · 2024-11-20T11:59:05Z

Sure, I will do that.

DJMcNab

This is looking good and runs as expected. Thanks for pulling this together

The exact integration with scenes could be cleaner, given that there's only one scene; if my guidance isn't clear enough, let me know and I can apply it myself.

DJMcNab · 2024-11-26T11:19:14Z

examples/scenes/src/cosmic_text_scene.rs

+
+        for (font_id, index) in font_faces {
+            if let Some(font) = font_system.get_font(font_id) {
+                let resource = Arc::new(font.data().to_vec());


There are two allocations here, one to put the data in the vec, and one to allocate the Arc. The first could be avoided by creating an adapter which implements AsRef<[u8]> by containing an Arc<Font> and using the data parameter. The second would use https://docs.rs/bytemuck/latest/bytemuck/allocation/trait.TransparentWrapperAlloc.html#method.wrap_arc, and is definitely out-of-scope.

The best resolution to this would probably be to have a comment: "These allocations are avoidable, but used for simplicity.", maybe with a link to this comment?

Not in time for this patch, but improving this is also a design goal for splitting these resource handles out of Peniko. (linebender/peniko#68)

I have used an adapter and added a link to this comment.

A wrapper/adapter is fine, but we should probably just submit a PR to cosmic-text to make Font implement AsRef<[u8]>. Alternatively, the correct type can be obtained using unsafe { font_system.db().make_shared_face_data(font_id).unwrap() }.

examples/scenes/src/lib.rs

examples/scenes/src/test_scenes.rs

examples/scenes/src/cosmic_text_scene.rs

AustinMReppert · 2024-11-26T21:31:32Z

Thanks for the feedback. I will work on this tomorrow.

DJMcNab

What needs to happen to get this out of draft? It's looking quite good

examples/scenes/src/cosmic_text_scene.rs

DJMcNab · 2024-12-02T09:42:35Z

examples/scenes/src/cosmic_text_scene.rs

+            }
+        }
+
+        let text = "おはよう (ja) (ohayō) 🌅✨ (morning), こんにちは (ja) (konnichi wa) ☀️😊 (daytime), こんばんは (ja) (konban wa) 🌙🌟 (evening)";


For future reviewers:

Source: https://github.com/pop-os/cosmic-text/blob/1f4065c1c3399efad58841082212f7c039b58480/sample/hello.txt#L113

These translations do also appear to be correct.

DJMcNab · 2024-12-02T09:44:03Z

examples/headless/Cargo.toml

@@ -20,3 +20,6 @@ pollster = { workspace = true }
 env_logger = "0.11.5"
 png = "0.17.14"
 futures-intrusive = { workspace = true }
+
+[features]
+cosmic_text = ["scenes/cosmic_text"]


Our style is to have a final newline at the end of all files.
(This also applies to the other files where you've chosen to omit this.)

This appears to have re-regressed in the same place.

examples/scenes/src/cosmic_text_scene.rs

DJMcNab

Would you be able to also document that this exists at the top level of the repository, in https://github.com/linebender/vello?tab=readme-ov-file#integrations

DJMcNab · 2024-12-02T11:51:50Z

examples/scenes/src/cosmic_text_scene.rs

+}
+
+// Copied directly from https://github.com/pop-os/cosmic-text/blob/58c2ccd1fb3daf0abc792f9dd52b5766b7125ccd/src/edit/editor.rs#L66
+fn cursor_position(cursor: &Cursor, run: &LayoutRun) -> Option<(i32, i32)> {


I think this is exposed via https://docs.rs/cosmic-text/latest/cosmic_text/struct.Editor.html#method.cursor_position; I'd much prefer we used that if at all feasible.

That function only returns the first cursor. I think the drawing assumes that there may be multi-line cursors. Although, the editor currently only has one cursor.

The reason I want that is for licensing. In the form you've currently included it, we would need to include the full System76 MIT license somewhere (or use this section of code only under the Apache 2.0 license, probably moving it into its own file).

examples/scenes/src/cosmic_text_scene.rs

waywardmonkeys · 2024-12-10T14:10:20Z

Is there anything left here other than what I just commented on? Or is this basically ready to go? It would be nice to land it soon!

AustinMReppert · 2024-12-11T16:16:07Z

I will rebase this hopefully in the next day or two

DJMcNab

This is still blocked on the licensing question for the newly imported code - I'd still prefer us to just avoid importing that code entirely, but I might be missing something.

DJMcNab · 2024-12-16T09:26:56Z

README.md

@@ -146,6 +146,15 @@ A separate Linebender integration for playing Lottie animations is available thr

 A separate Linebender integration for rendering raw scenes or Lottie and SVG files in [Bevy] through [`bevy_vello`](https://github.com/linebender/bevy_vello).

+### Cosmic Text
+
+An example scene demonstrating the integration of COSMIC text for font loading and text layout through [COSMIC Text Scene](https://github.com/linebender/vello/blob/main/examples/scenes/src/cosmic_text_scene.rs).


This should probably use a relative path (so that e.g. people viewing this in their editor get the right path). It also lets us double check the link before merging.
See e.g. the links to the license files at the bottom of this file.

I do see that there are other cases in this file where that isn't done - it's fine not to fix that.

DJMcNab · 2024-12-16T09:27:32Z

examples/headless/Cargo.toml

@@ -20,3 +20,6 @@ pollster = { workspace = true }
 env_logger = "0.11.5"
 png = "0.17.14"
 futures-intrusive = { workspace = true }
+
+[features]
+cosmic_text = ["scenes/cosmic_text"]


This appears to have re-regressed in the same place.

DJMcNab · 2024-12-16T09:29:39Z

examples/scenes/src/cosmic_text_scene.rs

+}
+
+// Copied directly from https://github.com/pop-os/cosmic-text/blob/58c2ccd1fb3daf0abc792f9dd52b5766b7125ccd/src/edit/editor.rs#L66
+fn cursor_position(cursor: &Cursor, run: &LayoutRun) -> Option<(i32, i32)> {


The reason I want that is for licensing. In the form you've currently included it, we would need to include the full System76 MIT license somewhere (or use this section of code only under the Apache 2.0 license, probably moving it into its own file).

DJMcNab · 2024-12-16T09:30:46Z

vello_tests/Cargo.toml

@@ -22,3 +22,6 @@ nv-flip = "0.1.2"
 image = { workspace = true, features = ["png"] }

 scenes = { workspace = true }
+
+[features]
+cosmic_text = ["scenes/cosmic_text"]


DJMcNab linked an issue Nov 20, 2024 that may be closed by this pull request

Cosmic Text example #306

Open

waywardmonkeys added this to the Vello 0.4 Release milestone Nov 22, 2024

AustinMReppert marked this pull request as draft November 24, 2024 03:44

DJMcNab reviewed Nov 26, 2024

View reviewed changes

AustinMReppert requested a review from DJMcNab December 1, 2024 17:38

DJMcNab reviewed Dec 2, 2024

View reviewed changes

AustinMReppert marked this pull request as ready for review December 2, 2024 11:20

DJMcNab reviewed Dec 2, 2024

View reviewed changes

waywardmonkeys reviewed Dec 2, 2024

View reviewed changes

examples/scenes/src/cosmic_text_scene.rs Outdated Show resolved Hide resolved

examples/scenes/src/cosmic_text_scene.rs Outdated Show resolved Hide resolved

examples/scenes/src/cosmic_text_scene.rs Outdated Show resolved Hide resolved

nicoburns added the documentation Improvements or additions to documentation label Dec 12, 2024

AustinMReppert added 2 commits December 14, 2024 10:08

Add cosmic text example on top of main.

05d9096

Update to new colors

0a1ebba

AustinMReppert force-pushed the feature/cosmic-text-example branch from 14b9023 to 0a1ebba Compare December 14, 2024 15:27

DJMcNab reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cosmic text example #739

Add cosmic text example #739

AustinMReppert commented Nov 19, 2024

DJMcNab commented Nov 20, 2024

AustinMReppert commented Nov 20, 2024

DJMcNab left a comment

DJMcNab Nov 26, 2024

waywardmonkeys Dec 1, 2024

AustinMReppert Dec 1, 2024

nicoburns Dec 4, 2024

AustinMReppert commented Nov 26, 2024

DJMcNab left a comment •

edited

Loading

DJMcNab Dec 2, 2024

DJMcNab Dec 2, 2024

DJMcNab Dec 16, 2024

DJMcNab left a comment

DJMcNab Dec 2, 2024

AustinMReppert Dec 14, 2024 •

edited

Loading

DJMcNab Dec 16, 2024

waywardmonkeys commented Dec 10, 2024

AustinMReppert commented Dec 11, 2024

DJMcNab left a comment

DJMcNab Dec 16, 2024

DJMcNab Dec 16, 2024

DJMcNab Dec 16, 2024

DJMcNab Dec 16, 2024

Add cosmic text example #739

Are you sure you want to change the base?

Add cosmic text example #739

Conversation

AustinMReppert commented Nov 19, 2024

DJMcNab commented Nov 20, 2024

AustinMReppert commented Nov 20, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AustinMReppert commented Nov 26, 2024

DJMcNab left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AustinMReppert Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waywardmonkeys commented Dec 10, 2024

AustinMReppert commented Dec 11, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab left a comment •

edited

Loading

AustinMReppert Dec 14, 2024 •

edited

Loading