pop-os / cosmic-text

Pure Rust multi-line text handling
https://pop-os.github.io/cosmic-text/cosmic_text/
Apache License 2.0
1.56k stars 95 forks source link

BIDI Layout is "random" #113

Open CryZe opened 1 year ago

CryZe commented 1 year ago

Depending on whether it's a mixed layout or not, the arabic characters are either reversed or not:

https://i.imgur.com/WNUdgIy.png

This is with the latest master.

Here you see a debugger view of just the arabic characters:

https://i.imgur.com/scFc9kv.png

This may or may not be intended, but is super weird to deal with. To properly handle this I need to properly mix the shape line's BIDI level with the shape span's BIDI level and then depending on what combination it is, iterate the glyphs in reverse or not.

I don't even understand why cosmic-text (sometimes) reverses what rustybuzz spits out in the first place, as I need to unreverse it to draw the glyphs in the proper order. That seems like a lot of wasted work that cosmic-text does.

hojjatabdollahi commented 1 year ago

Would you please share the text file that causes this issue?

CryZe commented 1 year ago

First line: Mixed اَلْعَرَبِيَّةُ BIDI Second line: اَلْعَرَبِيَّةُ

hojjatabdollahi commented 1 year ago

Looks correct in both editor-libcosmic and editor-orbclient. image

I'm guessing if you add some English text to the second line it would be reversed too. Is the algorithm you're using to render these somewhere that I can see?

CryZe commented 1 year ago

Yeah I'm not surprised that it looks correct in its own renderer. The thing is rather that internally the BIDI is all mangled as shown, you can probably very easily verify this with your own debugger. So this is either a case of multiple bugs cancelling each other out, or it's a really weird design decision that is unnecessarily complicated and slow.

The difference is probably that in the second, the line itself is right to left, rather than just the span, whereas the first line is left to right, so all glyphs probably get drawn in opposite order in the second line, cancelling out the problem.

hojjatabdollahi commented 1 year ago

I need to unreverse it to draw the glyphs

I think the problem lies here. As far as I know, you don't need to reverse the glyphs, just iterate through them and draw them. The renderer doesn't need to know if the text is RTL or LTR.

CryZe commented 1 year ago

You 100% do, but further debugging revealed that ONLY the line's RTL / LTR influences the iteration order, as every glyph is reversed then, even those for LTR spans in there. So if you have a span of Foo within a RTL line, it's stored as ooF. So iterating it in that order and drawing it would definitely result in the wrong glyph order.

I'm guessing that you are not meant to iterate in opposite order, as you are right, this does not seem correct, but that's necessary at the moment. But somehow cosmic-text calls slice::reverse(), to probably fix a problem in the layout algorithm, rather than fixing the layout algorithm itself.

Here the problematic lines straight from the source: https://github.com/pop-os/cosmic-text/blob/6c355bf08bbe0e1a513e53b4e192d821dc9aa552/src/shape.rs#L404-L409

dhardy commented 1 year ago

So iterating it in that order and drawing it would definitely result in the wrong glyph order.

Visually it shouldn't because the glyph positions are encoded within the iterator. The only time the iterator order should matter is when trying to handle left/right arrow keys to move the text edit cursor.

In kas-text I just specified that glyph iteration has undefined order. Visually it still works fine.

CryZe commented 1 year ago

Visually it shouldn't because the glyph positions are encoded within the iterator.

What iterator are you using? I'm just iterating over the shape lines, which essentially is just a bunch of layers of Vecs and those do not contain any positions, only x/y advances (and word x/y advances). Those are always positive, so if the glyphs are stored as [o, o, F] for the word Foo in the Vec and the x advances are always positive, then I have no choice other than iterating the Vec backwards.

hojjatabdollahi commented 1 year ago

You give your font and text to ShapeLine::new() and get your layout out of ShapeLine::layout(). That's all you need to do. You can just iterate through LayoutLines and LayoutGlyphs and render them.

Or you can just use Buffer.

Or take a look at the simple example in the docs.

jackpot51 commented 1 year ago

@CryZe Providing an example of how you are interfacing with cosmic-text will be required.

CryZe commented 1 year ago
use cosmic_text::{Attrs, AttrsList, BufferLine, FontSystem};

fn main() {
    let mut font_system = FontSystem::new();
    let mut line = BufferLine::new("اَلْعَرَبِيَّةُ foo اَلْعَرَبِيَّةُ", AttrsList::new(Attrs::new()));
    let shape_line = line.shape(&mut font_system);
    for glyph in &shape_line.spans[1].words[0].glyphs {
        dbg!(glyph.x_advance, glyph.glyph_id);
    }
}

This prints (annotated)

[src\main.rs:8] glyph.x_advance = 0.584 // That's going to the right
[src\main.rs:8] glyph.glyph_id = 555  // That's an `o`
[src\main.rs:8] glyph.x_advance = 0.584 // That's going to the right
[src\main.rs:8] glyph.glyph_id = 555  // That's an `o`   
[src\main.rs:8] glyph.x_advance = 0.325 // That's going to the right
[src\main.rs:8] glyph.glyph_id = 450  // That's an `f`

I'm not particularly interested in Buffer or the additional layout step after the shaping, as I'm not using any of the editing functionality, nor do I require multiple lines, and thus none of the line breaking or other parts of the layout algorithm.

What comes out of the layout algorithm is probably correct, as opposed to the shaping step, but I'm still thinking it's more of an accident where the layout algorithm itself undoes the reverse step.

hojjatabdollahi commented 1 year ago

Since the title says "BIDI Layout is random", I thought you are having issues with the layout algorithm. I didn't know you're doing your own layouting. Text layout is complex. BiDi layout is 10x more complex. The whole point of cosmic-text is that it does that for you! Otherwise there are many other libraries that just provide shaping (such as rustybuzz that cosmic-text is using).

That being said, I'm sure it's possible to move the reversing of words and glyphs from ShapeSpan::new() to ShapeLine::layout() with some elbow grease and a whole lot of testing.

I'm not particularly interested in Buffer or the additional layout step after the shaping, as I'm not using any of the editing functionality, nor do I require multiple lines, and thus none of the line breaking or other parts of the layout algorithm.

Even if you're doing one line of text, you still need layouting for BiDi text. Take a look at this.