Implement inline box layout #67

nicoburns · 2024-05-24T17:41:36Z

Objective

Resolve Support inline box layout #25

Changes made

The concept of a "layout item" has been introduced. This is either a run of text or an inline box.

DJMcNab

A scanning code-style review. Some points here probably aren't related to your PR, but happen to be in your diff due to new indentation - feel free to ignore

examples/swash_render/src/main.rs

DJMcNab · 2024-06-12T07:30:10Z

parley/src/context.rs

@@ -135,7 +144,8 @@ impl<'a, B: Brush, T: TextSource> RangedBuilder<'a, B, T> {
                char_index += 1;
            }
        }
-        use super::layout::{Decoration, Style};
+
+        // Define a function that converts `ResolvedDecoration` into `Decoration` (used just below)


I'm not sure how useful this comment is - maybe it would be better to rename the function?

Might be better as an impl on Decoration<B> since it is unrelated to LayoutContext and doesn't access self at all.
And just as a matter of style, the words ‘define a function that...’ don't belong in a comment right above a fn definition.

parley/src/context.rs

DJMcNab · 2024-06-12T07:33:37Z

parley/src/inline_box.rs

+    /// User-specified identifier for the box, which can be used by the user to determine which box in
+    /// parley's output corresponds to which box in it's input.
+    pub id: u64,
+    /// The character index into the underlying text string at which the box should be placed.


I'm presuming this is a byte index

Yes. Docs updated to reflect this.

DJMcNab · 2024-06-12T07:34:16Z

parley/src/inline_box.rs

+    /// User-specified identifier for the box, which can be used by the user to determine which box in
+    /// parley's output corresponds to which box in it's input.
+    pub id: u64,
+    /// The character index into the underlying text string at which the box should be placed.


We should document the bidi decision here (i.e. that the boxes follow the bidi of the logically previous character)

parley/src/layout/data.rs

DJMcNab · 2024-06-12T07:37:21Z

parley/src/layout/data.rs

+            }
+            LayoutItemKind::InlineBox => {
+                // TODO: account for vertical alignment (e.g. baseline alignment)
+                layout.inline_boxes[self.index].height


So we don't implement flowing around the inline boxes; that makes sense - it seems like it would be a nightmare to do so.

But we should probably make sure that's documented somewhere

parley/src/layout/line/greedy.rs

DJMcNab · 2024-06-12T07:50:41Z

parley/src/layout/line/greedy.rs

+                                    self.state.cluster_idx += 1;
+                                }
+                                if try_commit_line!(BreakReason::Emergency) {
+                                    self.state.cluster_idx += 1;


I've not tested it, but it seems like this would always skip a cluster? This doesn't look the same as other code above (but be aware that I'm only doing a single-pass review on unfamiliar code)

Good question. This appears to match the original code which seemed to handle this correctly.

I would also like to understand how/why/if this works correctly (and I don't). But as Chad says, this code is unchanged from the existing code so I don't think it should be blocking.

dfrg

Thanks Nico! It's really exciting to see this actually working even if it's not quite complete.

I left a lot of notes, some of which are suggestions and some are questions about how the changes here might play with future work.

Hopefully we can resolve some of the open questions and move this out of draft soon.

dfrg · 2024-06-16T14:19:37Z

parley/src/context.rs

+    pub fn push_inline_box(&mut self, inline_box: InlineBox) {
+        self.lcx.inline_boxes.push(inline_box);
+    }


There's a trade off here in how we split the complexity. I assume this requires sorting the boxes later and then carefully keeping track of them in the shaping stage.

Thinking ahead to generalizing to tree based layout, I'd probably prefer to track/split item ranges here during building. Basically, context should probably keep a Vec<Element> where Element is something like:

enum Element { Text(Range<usize>), // (byte offset, box properties) InlineBox(usize, InlineBox), // maybe others for more complicated layout: StartSpan, EndSpan, Break, etc. }

My thinking is that we initialize the vec with Element::Text containing the whole range. When pushing an object, we binary search for the appropriate insertion point and then split any overlapping text range. This introduces some additional complexity here while reducing it in later stages. It also maps more closely to what we might generate when building from a tree and lets us deal with object locations that might not land on character boundaries.

If this seems like a reasonable approach to you, I think it makes sense to push it to a future PR.

Seems reasonable. Although it might potentially make the bidi resolution stage a little more tricky?

My guess is that bidi resolution is going to become more complicated anyway if we want to properly handle inline objects, borders, margin, padding, etc.

parley/src/layout/line/greedy.rs

dfrg · 2024-06-16T15:11:18Z

parley/src/layout/line/greedy.rs

+                        // We only skip a mandatory break immediately after another mandatory break so if we
+                        // have got this far then we should disable mandatory break skipping
+                        self.state.line.skip_mandatory_break = false;


I think the actual reasoning here is that we return early from the function when processing a mandatory break that we actually handle (skip_mandatory_break == false).

I'll try to do a state table to capture the logic when boundary == Mandatory:

skip_mandatory_break action

false Commit line without consuming newline cluster, set skip_mandatory_break to true, return. Next pass will process this cluster again.

true Pretend like this isn't a break and append the newline cluster to the current line (this will always be the beginning of the line).

The key point is that mandatory break clusters are processed twice.

Hmm... I see. Could we change this so that we eagerly append the cluster to the newline instead? (can of course be in a followup PR)

Yes, we can definitely do better here. Incidentally, I tried changing the code so the shaper forces a break before every newline cluster and found that it already does so after one, so the code appears to be working at competing purposes here. Let’s address this later.

parley/src/layout/line/greedy.rs

parley/src/layout/line/mod.rs

dfrg · 2024-06-17T03:43:45Z

parley/src/shape.rs

+        while let Some(box_idx) = deferred_boxes.pop() {
+            // Push the box to the list of items
+            layout.data.push_inline_box(box_idx);
+        }


I think this will reverse the order of inline boxes with the same index.

The deferred_boxes vec could be replaced by an Option<Range<usize>> since they're guaranteed to be contiguous.

Good catch! Using a range is also a good idea.

Ok. I've updated this to:

Use .drain() to fix the ordering issue

Hoist the Vec allocation out of the loop so it's now only 1 Vec alloc per call to shape_text

I'm hoping the range based version can be tackled in a followup.

This allows consumers of Parley to set the size of boxes before running layout.

…eaking up runs

nicoburns · 2024-07-20T09:07:23Z

@dfrg This is now ready for re-review. I've cleaned up the history and backported all the fixes to line breaking that were previously only included in #76

If you're open to merging and then improving in followups then that would be my preferred approach as there are quite a few further improvements to parley in general that I'd like to make, but I'm currently a little wary that the stack of PRs is getting quite long and unwieldy.

xorgy

On this pass it looks mostly good. I may do one more detailed review.

xorgy · 2024-07-20T12:46:27Z

parley/src/context.rs

@@ -135,7 +144,8 @@ impl<'a, B: Brush, T: TextSource> RangedBuilder<'a, B, T> {
                char_index += 1;
            }
        }
-        use super::layout::{Decoration, Style};
+
+        // Define a function that converts `ResolvedDecoration` into `Decoration` (used just below)


Might be better as an impl on Decoration<B> since it is unrelated to LayoutContext and doesn't access self at all.
And just as a matter of style, the words ‘define a function that...’ don't belong in a comment right above a fn definition.

xorgy · 2024-07-20T12:52:04Z

parley/src/context.rs

+
+        // Sort the inline boxes as subsequent code assumes that they are in text index order.
+        // Note: It's important that this is a stable sort to allow users to control the order of contiguous inline boxes
+        lcx.inline_boxes.sort_by_key(|b| b.index);


Should we use an inherently sorted structure instead of Vec if the code assumes they're sorted? Memory is a good reason not to, but just a thought.

For better or worse, parley was intentionally designed to avoid transient heap allocation and a non-trivial chunk of the complexity is a result of that. So I’d like to avoid letting “allocation per item” collections creep in and break that property. There are already a couple cases of that in this PR (including the stable sort here) but I believe they will all be corrected in future work.

I mean, it doesn't have to be BTreeMap, but I guess that's one more dependency.

xorgy · 2024-07-20T12:56:14Z

parley/src/layout/data.rs

+    pub advance: f32,
+
+    // Fields that only apply to text runs (Ignored for boxes)
+    // TODO: factor this out?


Maybe instead of kind this could be factored into an enum that contains this stuff.. though arguably text_range still applies (it's just a caret range) and ditto cluster_range and is_whitespace.

+1 to rethinking some of these data structures but that’s likely to affect a lot of code so I’d like to see it done as a follow up.

dfrg

While there are still some rough edges, this a great step forward and I’m inclined to agree with merging this now to unblock future progress. Thanks Nico!

Suggestions either addressed or Chad has suggested we do later

nicoburns force-pushed the inline-boxes branch from 31fb7ea to ce04065 Compare May 30, 2024 15:12

nicoburns mentioned this pull request Jun 2, 2024

Remove taffy upgrade from release notes bevyengine/bevy-website#1208

Merged

nicoburns force-pushed the inline-boxes branch 2 times, most recently from a87c34d to acccac3 Compare June 8, 2024 04:59

xorgy self-requested a review June 11, 2024 03:04

nicoburns mentioned this pull request Jun 12, 2024

Tree style builder #76

Open

DJMcNab reviewed Jun 12, 2024

View reviewed changes

dfrg reviewed Jun 17, 2024

View reviewed changes

nicoburns force-pushed the inline-boxes branch 12 times, most recently from 72f9d0e to 9976cf1 Compare July 20, 2024 08:55

nicoburns added 9 commits July 20, 2024 20:59

Add inline boxes to the Layout and RangedBuilder

9278b1b

Allow (mutable) access to inline boxes within a layout

176f1df

This allows consumers of Parley to set the size of boxes before running layout.

Add items field to LayoutData and push inline boxes to it when br…

c410b40

…eaking up runs

Add bidi_level field to LayoutItem

4f0c0e1

Convert LineRunData to LineItemData

06b9a27

Refactor alignment code

41b128e

Implement line-breaking for inline boxes

f92b2af

Non-emergency linebreaking

b7ba3f0

Separate alignment from line breaking

1f666d2

nicoburns force-pushed the inline-boxes branch from 9976cf1 to 92a4012 Compare July 20, 2024 09:00

Update examples for inline boxes

f1e1e9b

nicoburns force-pushed the inline-boxes branch from 92a4012 to f1e1e9b Compare July 20, 2024 09:01

nicoburns changed the title ~~WIP: Implement inline box layout~~ Implement inline box layout Jul 20, 2024

nicoburns marked this pull request as ready for review July 20, 2024 09:02

nicoburns mentioned this pull request Jul 20, 2024

Use CSS definition of line height #84

Merged

xorgy previously requested changes Jul 20, 2024

View reviewed changes

dfrg approved these changes Jul 20, 2024

View reviewed changes

Convert conv_devo function into a method

ae95c49

nicoburns force-pushed the inline-boxes branch from f7405ed to 8ffa1ac Compare July 20, 2024 22:14

Convert style conversion in a method

74cff35

nicoburns force-pushed the inline-boxes branch from 8ffa1ac to 74cff35 Compare July 20, 2024 22:14

nicoburns added this pull request to the merge queue Jul 20, 2024

Merged via the queue into linebender:main with commit caabcf1 Jul 20, 2024
19 checks passed

nicoburns deleted the inline-boxes branch July 20, 2024 22:23

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement inline box layout #67

Implement inline box layout #67

nicoburns commented May 24, 2024 •

edited by xorgy

Loading

DJMcNab left a comment •

edited

Loading

DJMcNab Jun 12, 2024

xorgy Jul 20, 2024

DJMcNab Jun 12, 2024

nicoburns Jul 20, 2024

DJMcNab Jun 12, 2024

DJMcNab Jun 12, 2024

DJMcNab Jun 12, 2024

dfrg Jun 16, 2024

nicoburns Jul 20, 2024

dfrg left a comment

dfrg Jun 16, 2024 •

edited

Loading

nicoburns Jul 20, 2024

dfrg Jul 20, 2024

dfrg Jun 16, 2024

nicoburns Jul 20, 2024

dfrg Jul 20, 2024

dfrg Jun 17, 2024

nicoburns Jul 20, 2024

nicoburns Jul 20, 2024

nicoburns commented Jul 20, 2024

xorgy left a comment •

edited

Loading

xorgy Jul 20, 2024

xorgy Jul 20, 2024

dfrg Jul 20, 2024

xorgy Jul 20, 2024

xorgy Jul 20, 2024

dfrg Jul 20, 2024

dfrg left a comment

skip_mandatory_break	action
false	Commit line without consuming newline cluster, set skip_mandatory_break to true, return. Next pass will process this cluster again.
true	Pretend like this isn't a break and append the newline cluster to the current line (this will always be the beginning of the line).

Implement inline box layout #67

Implement inline box layout #67

Conversation

nicoburns commented May 24, 2024 • edited by xorgy Loading

Objective

Changes made

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

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

dfrg left a comment

Choose a reason for hiding this comment

dfrg Jun 16, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoburns commented Jul 20, 2024

xorgy 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

Choose a reason for hiding this comment

dfrg left a comment

Choose a reason for hiding this comment

nicoburns commented May 24, 2024 •

edited by xorgy

Loading

DJMcNab left a comment •

edited

Loading

dfrg Jun 16, 2024 •

edited

Loading

xorgy left a comment •

edited

Loading