cosmic-text icon indicating copy to clipboard operation
cosmic-text copied to clipboard

BIDI Layout is "random"

Open CryZe opened this issue 1 year ago • 12 comments

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.

CryZe avatar Mar 26 '23 14:03 CryZe

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

hojjatabdollahi avatar Mar 26 '23 14:03 hojjatabdollahi

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

CryZe avatar Mar 26 '23 14:03 CryZe

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?

hojjatabdollahi avatar Mar 26 '23 14:03 hojjatabdollahi

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.

CryZe avatar Mar 26 '23 14:03 CryZe

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.

hojjatabdollahi avatar Mar 26 '23 16:03 hojjatabdollahi

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

CryZe avatar Mar 26 '23 16:03 CryZe

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.

dhardy avatar Mar 26 '23 16:03 dhardy

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.

CryZe avatar Mar 26 '23 17:03 CryZe

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.

hojjatabdollahi avatar Mar 26 '23 17:03 hojjatabdollahi

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

jackpot51 avatar Mar 26 '23 17:03 jackpot51

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.

CryZe avatar Mar 26 '23 18:03 CryZe

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.

hojjatabdollahi avatar Mar 26 '23 21:03 hojjatabdollahi