glyphon icon indicating copy to clipboard operation
glyphon copied to clipboard

Improve API ergonomics

Open grovesNL opened this issue 2 years ago • 1 comments

It's a bit difficult to organize fonts/layouts and pass them to renderer.

Consider some other way to handle fonts, layouts, etc., possibly wrapping all of fontdue to remove fontdue from the public API.

grovesNL avatar May 10 '22 11:05 grovesNL

Also would like to find a better way to handle the current HasColor/Color setup

grovesNL avatar May 10 '22 12:05 grovesNL

+1 for a better color setup, once I give the color to layout.append, I don't see a way to change it without clear+append, which is very wasteful even if fontdue is quite fast.

and a Layout of a text widget has to belong to the widget (for updating it when the widget changes) but I need a Vec<Layout> in my renderer for lending it to text_renderer.prepare after I laid out all my widgets, .prepare() which won't accept a &[&Layout] or something GlyphPosition-based, even through it apparently only does .iter().glyphs() on it, iterates it again and just immutably uses the result And Layout is not clonable and I don't want to figure out how to pass a collection of Rc to .prepare()

glyphon also needs to be clearer about its limitations and provide more examples than hello world

m-hugo avatar Oct 20 '22 13:10 m-hugo

I made .prepare() take a layouts: &[MyLayout] instead, MyLayout is cheaply built in my widgets' draw functions every redraw from its stored Layout

  • I can choose the color I want (in .draw() itself, without the widget's .layout()'s repositioning from .append())
  • the only thing that gets cloned is the Vec<GlyphPosition> output of .glyphs()
pub struct MyLayout{
    pub glyphs: Vec<GlyphPosition>,
    pub teov: TextOverflow,
    pub minmax: (u32, u32, u32, u32),
    pub color: Color,
}

I also made MyLayout clonable even though I didn't need to since it was just #[derive(Clone)] on MyLayout, Color, TextOverflow

m-hugo avatar Oct 21 '22 02:10 m-hugo

@genusistimelord @grovesNL what do you think of this MyLayout, does one of you want to make a nice PR for it or should I do it myself (I hate git and github)?

ps: minmax is ( bounds_min_x, bounds_max_x, bounds_min_y, bounds_max_y, )

m-hugo avatar Oct 21 '22 02:10 m-hugo

@m-hugo Hi! 👋 Thanks for the questions about color/layout interactions.

+1 for a better color setup, once I give the color to layout.append, I don't see a way to change it without clear+append, which is very wasteful even if fontdue is quite fast.

It shouldn't be necessary to clear and append the layout again, although you could if it's easier to do it that way. For what it's worth, it shouldn't be too wasteful on the fontdue side because the rasterized glyphs will usually already be cached from the last round of calls to prepare/render.

Instead of clearing and appending, you could have a field containing Rc<ColorLookupThing> on your own GlyphUserData, then use ColorLookupThing in the implementation of the HasColor trait to select a color. During prepare and render, glyphon will call color to get whatever the latest color value should be.

what do you think of this MyLayout

I considered wrapping fontdue's Layout like this initially, but ended up trying to use fontdue's Layout type directly as long as possible (i.e., if we can keep it simple enough to do it ergonomically). I think it might be nice to expose some kind of builder or something that makes it easier to attach colors to TextStyles though.

It should already be possible to set the color during your draw using the trait though, so I'd be interested if you could share an example of the part that's causing problems in your project.

grovesNL avatar Oct 21 '22 03:10 grovesNL

Rc<ColorLookupThing> on your own GlyphUserData, then use ColorLookupThing in the implementation of the HasColor trait to select a color.

that would work, it's just so twisted I didn't even think of it...

and how are you supposed to give ownership of the Layout to a Vec for prepare because it's not clone (and it would clone a lot of unneeded data if it were), I'm not sure how to give a Vec<Rc<Layout>> to prepare either...

m-hugo avatar Oct 21 '22 03:10 m-hugo

In my project I just have a field like layouts: Vec<(Layout<GlyphUserData>, TextOverflow)> somewhere that I push all layouts into (e.g., collecting text from all widgets), then pass &self.layouts[..layouts_used] to prepare.

In that kind of set up, GlyphUserData could still have a Rc<ColorLookupThing> internally but the layout Vec or Layouts don't need to be Rc in that case.

grovesNL avatar Oct 21 '22 03:10 grovesNL

pub struct Renderer {
    pub layouts: Vec<MyLayout>,
    text_renderer: TextRenderer,
   ...
}

   pub fn redraw(&mut self) {
        self.text_renderer
            .prepare(
                &self.device,
                &self.queue,
                &mut self.atlas,
                Resolution {
                    width: self.config.width,
                    height: self.config.height,
                },
                &self.fonts,
                &self.layouts,
            )
            .unwrap();
       ...
   }


       Widget::Text(s, fonts, font_size, layout, color) => {
                let settings = layout.settings();
                let bounds_min_x = settings.x.trunc() as u32;
                let bounds_max_x = settings
                    .max_width
                    .map(|w| bounds_min_x + w.trunc() as u32)
                    .unwrap_or(i32::MAX);
                let bounds_min_y = settings.y.trunc() as u32;
                let bounds_max_y = settings
                    .max_height
                    .map(|h| bounds_min_y + h.trunc() as u32)
                    .unwrap_or(i32::MAX);

                let ml = MyLayout {
                    minmax: (bounds_min_x, bounds_max_x, bounds_min_y, bounds_max_y),
                    color: color.clone(),
                    glyphs: layout.glyphs().to_vec(),
                    teov: TextOverflow::Hide,
                };
                renderer.layouts.push(ml);
            }

this works, now if I do renderer.layouts.push(layout), it will fail because layout is &Layout not Layout

m-hugo avatar Oct 21 '22 03:10 m-hugo

do you send the Layout to the Vec directly after appending all your text? then how do you change the text of one layout without clearing the whole vec and building it again (clearing and appending all the Layouts in the process)

m-hugo avatar Oct 21 '22 04:10 m-hugo

@m-hugo ah ok I think I understand your use case better now. Would the change in #21 work well for you? It should allow either (1 - the old behavior) &[layout, overflow] or (2 - your use case) &[&layout, overflow] to be provided

grovesNL avatar Oct 21 '22 11:10 grovesNL

it's better than before but not as nice as passing the Vec<GlyphPosition> directly since you have to store the ref to Layout (add indirection or deal with lifetimes)

and the only thing you do with the Layout is to take the clonable Vec<GlyphPosition> contained in it

Layout is meant for layout, it shouldn't be passed to a renderer

m-hugo avatar Oct 21 '22 11:10 m-hugo

Yeah it could be convenient to take the glyph positions directly, but I don't want to force people to have to clone Vec<GlyphPosition> every time the layout changes.

We also need Settings from Layout (beside the glyphs) for the layout bounds during prepare, so at that point I think we'd probably end up duplicating a lot of Layouts internals anyway.

grovesNL avatar Oct 21 '22 12:10 grovesNL

pub struct MyLayout<'a>{
    pub glyphs: &'a [GlyphPosition],
    pub teov: TextOverflow,
    pub settings: &'a LayoutSettings,
    pub color: Color,
}

is a straight upgrade over master or pr

here's Layout's fields btw:

pub struct Layout<U: Copy + Clone = ()> {
    /// Marks if layout should be performed as if the Y axis is flipped (Positive Y incrementing
    /// down instead of up).
    flip: bool,
    /// Origin position. Left side of the region text is being laid out in.
    x: f32,
    /// Origin position. Top side of the region text is being laid out in.
    y: f32,
    /// A mask to filter only linebreak types being requested.
    wrap_mask: LinebreakData,
    /// The max width of the region text is being laid out in.
    max_width: f32,
    /// The max height of the region text is being laid out in.
    max_height: f32,
    /// A multiplier for how text fills unused vertical space.
    vertical_align: f32,
    /// A multiplier for how text fills unused horizontal space.
    horizontal_align: f32,
    /// The current height of all laid out text.
    height: f32,

    /// Finalized glyph state.
    output: Vec<GlyphPosition<U>>,
    /// Intermediate glyph state.
    glyphs: Vec<GlyphPosition<U>>,

    /// Linebreak state. Used to derive linebreaks from past glyphs.
    linebreaker: Linebreaker,
    /// The current highest priority linebreak (Hard > Soft > None).
    linebreak_prev: LinebreakData,
    /// The x position that the glyph that has the current highest priority linebreak status starts
    /// at.
    linebreak_pos: f32,
    /// The index of the glyph that has the current highest priority linebreak status. This glyph is
    /// the last glyph on a line if a linebreak is required.
    linebreak_idx: usize,

    /// Layout state of each line currently laid out. This always has at least 1 element.
    line_metrics: Vec<LinePosition>,
    /// The x position the next glyph starts at.
    current_pos: f32,
    /// The ceil(ascent) of the current style.
    current_ascent: f32,
    /// The ceil(descent) of the current style.
    current_descent: f32,
    /// The ceil(line_gap) of the current style.
    current_line_gap: f32,
    /// The ceil(new_line_size) of the current style.
    current_new_line: f32,
    /// The x position the current line starts at.
    start_pos: f32,
    /// The settings currently being used for layout.
    settings: LayoutSettings,
}

taking &output and &settings, is the only thing you take (&)Layout for

m-hugo avatar Oct 21 '22 13:10 m-hugo

Doesn't the 'a lifetime of MyLayout cause the same issues as storing &'a Layout though? i.e., I'm not sure why it helps to accept &layout.output and &layout.settings instead of &layout

grovesNL avatar Oct 21 '22 13:10 grovesNL

GlyphPosition and LayoutSettings are clone, the MyLayout can be created anytime from a fully-owned struct

m-hugo avatar Oct 21 '22 13:10 m-hugo

generally using References can cause issue's later on if you wanted to Thread Layout Generation. Normally Clone or Copy is what you would Want if you decided to use Threading to handle Multiple Layouts in one go. I can take a closer look at this later. I myself Handled this in a bit different way though as I keep track of the char and Color per character. Also I have not implemented the overflow stuff yet as I am unsure if that is better than clipping or if clipping is better. etc.

genusistimelord avatar Oct 21 '22 13:10 genusistimelord

If I understand correctly I think #21 should handle your use case as long as the lifetimes are set up right, but I might be missing something.

In general I'd prefer to find ways to make this more ergonomic without requiring Clone, to_vec, etc., because I really want to avoid calls like layout.glyphs().to_vec() that might be expensive and called per-frame.

grovesNL avatar Oct 21 '22 13:10 grovesNL

it doesn't.

  1. Layout stuff, put Vec<GlyphPosition> and LayoutSettings into MyOwnedLayout
  2. move MyOwnedLayout without worrying about lifetimes
  3. turn MyOwnedLayout into MyLayout<'a> and give it to prepare

and passing a MyLayout<'a> is never more expensive than Layout

m-hugo avatar Oct 21 '22 13:10 m-hugo

What prevents you from going directly from Layout to MyLayout<'a> (skipping steps 1 and 2)?

grovesNL avatar Oct 21 '22 13:10 grovesNL

for the overflow stuff with LayoutSettings, I think it's a bad idea because it assumes you always wand to render all the glyphs in your provided bounds if I'm using a scroller to scroll text from a layout I only want to render the glyphs inside the scroller's bounds

m-hugo avatar Oct 21 '22 13:10 m-hugo

What prevents you from going directly from Layout to MyLayout<'a> (skipping steps 1 and 2)?

things involving threads or callbacks or other not-borrow-friendly things

m-hugo avatar Oct 21 '22 13:10 m-hugo

for the overflow stuff with LayoutSettings, I think it's a bad idea because it assumes you always wand to render all the glyphs in your provided bounds

I think this should work ok already (it's how I use scrollers in my application too), e.g. the bottom text in the example is clipped and culled (which could be the bounds of a scroller)

image

things involving threads or callbacks or other not-borrow-friendly things

I'm not totally sure how the extra steps improve that situation (at least without cloning when moving between threads or callbacks), but maybe we could try to create a minimal example that shows how the current setup is problematic?

grovesNL avatar Oct 21 '22 14:10 grovesNL

(at least without cloning when moving between threads or callbacks)

I obviously want to clone

GlyphPosition and LayoutSettings are clone, the MyLayout can be created anytime from a fully-owned struct


(it's how I use scrollers in my application too)

Can you give some code ?

the bottom text in the example is clipped and culled (which could be the bounds of a scroller)

statically through LayoutSettings{y, max_height}

layout2.reset(&LayoutSettings {
                    x: 0.0,
                    y: 200.0,     <---You can't change this without layout2.reset
                    max_width: Some(200.0), 
                    max_height: Some(190.0),    <---You can't change this without layout2.reset
                    horizontal_align: HorizontalAlign::Center,
                    vertical_align: VerticalAlign::Middle,
                    ..LayoutSettings::default()
                });

m-hugo avatar Oct 21 '22 14:10 m-hugo

Right, for now I reset the layout whenever I need to change the position or dimensions of the content within the scroller. The layouts for scrollers tend to be small enough that this is probably just as expensive as trying to clip/cull later on the GPU (e.g., discarding, depth testing, multiple draw calls with individual scissor areas, or something else), so I think it's probably a reasonable default.

It also might be a little easier in my case. I have lots of tiny layouts within the scroller, so I just remove any layouts that won't overlap with the visible scroller region.

I think we could probably also support the way you mentioned eventually with another mode where we don't clip/cull based on the layout settings at all. In that case you'd probably provide your own bounds you to use (which wouldn't match the bounds of the layout).

grovesNL avatar Oct 21 '22 14:10 grovesNL

Some of my scrollers could show only <1% of a very fat layout, I could split big strings into smaller ones, make many layouts and only push the few that are not fully out-of -bounds, I'll see how expensive it is

m-hugo avatar Oct 21 '22 15:10 m-hugo

well... I should maybe have profiled your hello world example before starting to optimize my code, have you seen the insane amount of cpu cycles it uses ? I've seen more performant cpu-only text renderers, at least 30% of that time is spent between append and prepare, FOR A STATIC IMAGE, in RELEASE mode. Modify the example to show me a way to make the layout1's text blue when the cursor is in the y range of layout1 (WindowEvent::CursorMoved position.y in 0-layout1.height()) and back to yellow when it leaves it AND without doing an ungodly amount of unnecessary work, I should be able to make it flicker with my mouse without seeing my cpu heat up. At this point I honestly believe it'll be easier to make my own fontdue-wgpu crate than modify yours...

m-hugo avatar Oct 24 '22 03:10 m-hugo

basically the way I am currently doing this is

pub struct Text {
    /// glyph string layout.
    pub glyphs: Vec<Glyph>,
    /// Font PX size,
    pub px: f32,
    /// Font Index,
    pub font_index: usize,
    /// Layout settings for the Text.
    pub settings: LayoutSettings,
    /// Vertex array in bytes. Does not need to get changed except on Text update.
    /// This Holds the Created layout for rendering.
    pub bytes: Vec<u8>,
    /// The string index where rendering starts from.
    pub cursor: usize,
    //Position to Render the Text At based on its Bottom Left Corner.
    pub pos: [f32; 3],
    /// The system layout of the Text Appened
    layout: Layout,
    /// If the location or map array id changed. Rebuild the vertex buffer.
    pub changed: bool,
}

I keep track of everything little needed and Rebuild the Layout only when needed.

text.clear();
text.append(&format!("FPS: {}", fps));
text.build_layout(&state.fonts);

this allows me to append lots of text and do lots of changes to it before rebuilding the layout. Also since i set each glyph for my string to be handled like


pub struct Glyph {
    pub ch: char,
    pub color: FontColor,
}

I can change the color per each glyph Or the entire String in one go. without needing to Rebuild the entire layout. since on Render I only use the layout for its actual positioning Etc. I also use the changed to determine if i need to rebuild the Buffer Bytes on render loop. But generally if you contently Change lets say Text then you will always need to Re-layout which is CPU intensive. If you need to change the Color then you will always need to rebuild the Quads which again is CPU intensive. Text is by far the worst and slowest of all the renderers you can have. The way a lot of people would try to optimize this is to split the text into parts which each have their own layout and handlers. this way if something is updated it only updates that single word or adds a new word with new layout so it doesn't rebuild the entire blob of text. This is very hard to do though as you then need to handle a layout between layouts to correctly position them. Some people use the layout of Spaces to determine the layout distances and offsets for these.

genusistimelord avatar Oct 24 '22 12:10 genusistimelord

@m-hugo How are you profiling it? Something seems wrong there. If you glance through all of the logic in prepare and render, everything is pretty simple and typical for any atlas-based glyph painter. Anecdotally I've also been using glyphon in a text-heavy wasm application (where CPU performance is lowered) and I haven't noticed any performance issues like that.

Modify the example to show me a way to make the layout1's text blue when the cursor is in the y range of layout1 (WindowEvent::CursorMoved position.y in 0-layout1.height()) and back to yellow when it leaves it AND without doing an ungodly amount of unnecessary work, I should be able to make it flicker with my mouse without seeing my cpu heat up.

It looks like the Copy bound on fontdue user data currently prevents us from doing the Rc idea I mentioned above, but recreating the layout would still work fine for now. Recreating the layout should still be very fast because the rasterized glyphs are cached. It would be nice to improve this in the future so that Rc idea becomes possible. I added an example here: #22

grovesNL avatar Oct 24 '22 12:10 grovesNL

look at my example on how to not use the fontdue user data. as this is not a needed thing and if you can bypass it then it will be much nicer.

genusistimelord avatar Oct 24 '22 12:10 genusistimelord

@genusistimelord yeah that makes sense. I had considered wrapping all of fontdues types in something like this initially, but ending up trying to work with the types directly. I'd like to avoid wrappers as much as possible (mostly just to keep the scope of this crate down), though builders that generate the fontdue types could be interesting.

I think user data almost works perfectly for dynamic (i.e,. not requiring layout resets) per-glyph colors, and we could probably upstream a change to avoid the Copy bound somehow (e.g., maybe the user data design could be changed slightly so that it returns a Copy value instead of being Copy itself).

grovesNL avatar Oct 25 '22 00:10 grovesNL