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() }.

DJMcNab · 2024-11-26T11:36:11Z

examples/scenes/src/lib.rs

@@ -34,6 +40,8 @@ pub struct SceneParams<'a> {
    pub resolution: Option<Vec2>,
    pub base_color: Option<vello::peniko::Color>,
    pub complexity: usize,
+    #[cfg(feature = "cosmic_text")]
+    pub cosmic_text_scene_state: &'a CosmicTextSceneState,


This is underdocumented, so I can understand this being unclear, but test scenes can have state.

The main result of this is that that we can make this feature gate be "localised", by making the cosmic_text_scene::text_scene function return an impl TestScene, which will be retained.
For an example, see the MMark scene.

This does lead to some things which are unidiomatic, such as not sharing the fonts between the scenes, but I think for this peripheral functionality, keeping it as localised as possible is better than improving the efficiency.

DJMcNab · 2024-11-26T11:39:52Z

examples/scenes/src/test_scenes.rs

@@ -53,6 +61,7 @@ macro_rules! scene {
 }

 export_scenes!(
+    #[cfg(feature = "cosmic_text")] cosmic_text_scene(cosmic_text),


This can use:

Suggested change

#[cfg(feature = "cosmic_text")] cosmic_text_scene(cosmic_text),

#[cfg(feature = "cosmic_text")] cosmic_text_scene(crate::cosmic_text_scene::cosmic_text(), "cosmic text", false),

DJMcNab · 2024-11-26T11:42:18Z

examples/scenes/src/test_scenes.rs

@@ -53,6 +61,7 @@ macro_rules! scene {
 }

 export_scenes!(
+    #[cfg(feature = "cosmic_text")] cosmic_text_scene(cosmic_text),
    splash_with_tiger(impls::splash_with_tiger(), "splash_with_tiger", false),


The splash screen should still be the first screen, to show the controls to anyone.

DJMcNab · 2024-11-26T11:46:22Z

examples/scenes/src/cosmic_text_scene.rs

@@ -0,0 +1,425 @@
+// Copyright 2024 the Vello Authors
+// SPDX-License-Identifier: Apache-2.0 OR MIT
+


A tiny bit of documentation would be good:

Suggested change

//! An example integrating Cosmic text with Vello.

//! This renders a read-only snapshot of a simplified text editor using system fonts.

DJMcNab · 2024-11-26T11:47:41Z

examples/scenes/src/cosmic_text_scene.rs

+    buffer_glyphs
+}
+
+// Copied directly from cosmic_text.


Can we have a link to the source?

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

DJMcNab · 2024-12-02T09:37:51Z

examples/scenes/src/cosmic_text_scene.rs

+    }
+}
+
+pub struct CosmicTextSceneState {


"State" is rarely a meaningful suffix; I think this would be clearer if it were removed.

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.)

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

examples/scenes/src/cosmic_text_scene.rs

+use std::collections::HashMap;
+use std::sync::Arc;
+
+pub struct CosmicData {}


What is this for?

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.

waywardmonkeys · 2024-12-01T16:17:44Z

examples/scenes/src/cosmic_text_scene.rs

+            vello_fonts,
+            buffer_glyphs: create_glyphs_for_editor(
+                &editor,
+                Color::from_rgba8(255, 255, 255, 255),


use vello::peniko::color::palette and then these can be palette::css::WHITE, palette::css::RED, palette::css::BLUE, and palette::css::WHITE.

I'm working on fixing up other samples to do similar after the color changes.

use vello::peniko::color

Ew. We should really add color to the examples as a top-level dependency.

waywardmonkeys · 2024-12-02T11:55:36Z

examples/scenes/src/cosmic_text_scene.rs

+            };
+
+            if let Some(editor_info) = &editor_info {
+                if text_color.components != editor_info.selected_text_color.components {


This isn't the right way to compare, but I understand that it is the easier for now.

We're addressing this in linebender/color#76 but it'll take some time, so this can get fixed after something else lands and gets propagated here.

If you rebase on top of the current main as of a couple of minutes ago, the color now impl's PartialEq, so you can just if text_color != ... instead of looking at the components.

waywardmonkeys · 2024-12-02T11:55:44Z

examples/scenes/src/cosmic_text_scene.rs

+
+            if let Some((last_font, last_glyph_color)) = last_font {
+                if last_font != glyph.font_id
+                    || last_glyph_color.components != glyph_color.components


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

AustinMReppert added 4 commits November 19, 2024 18:05

Add cosmic text example.

1335c4e

Format.

99ea890

Fix clippy lints.

707a03b

Format.

4452503

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 added 3 commits November 23, 2024 10:05

Merge branch 'linebender:main' into feature/cosmic-text-example

eadaa91

Move cosmic example to test scenes and add editor.

81f9a29

Add cosmic_text_scene.rs

8a4fa99

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

AustinMReppert added 5 commits November 24, 2024 08:43

Add cosmic text feature to tests and fix clippy lint.

401c481

Add missing import.

baa4c5b

Add cosmic text feature to headless.

5852feb

Add copyright.

f333015

Ensure reported font exists.

fb43023

DJMcNab reviewed Nov 26, 2024

View reviewed changes

AustinMReppert added 5 commits December 1, 2024 10:38

Merge branch 'main' into feature/cosmic-text-example

a6732ab

Use scene state.

f92e2f3

Add docs

410b947

Add adapter for cosmic.

0e78992

Remove unused import.

f8a22af

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

DJMcNab reviewed Dec 2, 2024

View reviewed changes

Cleanup.

07d30e0

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

AustinMReppert added 2 commits December 2, 2024 08:27

Add cosmic text scene integration to README.

eebe5d5

Fix wording.

14b9023

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

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

DJMcNab Nov 26, 2024

AustinMReppert Dec 1, 2024

DJMcNab Nov 26, 2024

DJMcNab Nov 26, 2024

AustinMReppert Dec 1, 2024

DJMcNab Nov 26, 2024

AustinMReppert Dec 1, 2024

DJMcNab Nov 26, 2024

AustinMReppert Dec 1, 2024

AustinMReppert commented Nov 26, 2024

DJMcNab left a comment •

edited

Loading

DJMcNab Dec 2, 2024

DJMcNab Dec 2, 2024

DJMcNab Dec 2, 2024

DJMcNab Dec 2, 2024

AustinMReppert Dec 2, 2024

DJMcNab left a comment

DJMcNab Dec 2, 2024

waywardmonkeys Dec 1, 2024

nicoburns Dec 4, 2024

waywardmonkeys Dec 2, 2024

waywardmonkeys Dec 10, 2024

waywardmonkeys Dec 2, 2024

waywardmonkeys commented Dec 10, 2024

AustinMReppert commented Dec 11, 2024

	#[cfg(feature = "cosmic_text")] cosmic_text_scene(cosmic_text),
	#[cfg(feature = "cosmic_text")] cosmic_text_scene(crate::cosmic_text_scene::cosmic_text(), "cosmic text", false),

		@@ -0,0 +1,425 @@
		// Copyright 2024 the Vello Authors
		// SPDX-License-Identifier: Apache-2.0 OR MIT



	//! An example integrating Cosmic text with Vello.
	//! This renders a read-only snapshot of a simplified text editor using system fonts.

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

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

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

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

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

waywardmonkeys commented Dec 10, 2024

AustinMReppert commented Dec 11, 2024

DJMcNab left a comment •

edited

Loading