Move layout code into Rust #314

rylin8 · 2023-08-24T00:53:14Z

No description provided.

johnoneil · 2023-08-25T17:14:19Z

I only have superficial feedback on this so far, but it has been helpful in my starting to understand how we will use the Rust code here in our pure rust client code to manage layout.

In the layout crate the primary thing I'd like to see would be a pure Rust example that shows a kind of example integration. That is, how to use in a pure Rust environment.

I imagine we'd have to provide functions that stand in where existing .kt currently provides information, for example with text geometry.

Currently we have a (very basic, and static) layout implementation which is struggling with how to manage variants, so despite the difficulty here this code is very much needed. This is much appreciated Rob.

rylin8 · 2023-08-25T18:06:06Z

Honestly I haven't fully thought through how to integrate this with Rust and would like to have a conversation about that soon. However I have been testing the layout code purely in Rust in a number of scenarios and plan to add a bunch of unit tests. This should be a good starting point on how to use it. From there I'll try to add another level of abstraction for the Rust code to use, similar to the JNI interface used by Android.

iamralpht · 2023-08-28T05:38:31Z

@rylin8 I think the layout crate is missing; it's mentioned in the Cargo workspace decl, but isn't in the PR.

rylin8 · 2023-08-28T16:27:03Z

@rylin8 I think the layout crate is missing; it's mentioned in the Cargo workspace decl, but isn't in the PR.

Currently the layout crate isn't used. I made it initially and put all the layout code in it, then decided to use the live update crate because at the time, layout was tied to live update. Now I need to move it back to the other crate, after which I'll add the code back into this PR.

timothyfroehlich · 2023-08-31T16:03:09Z

This is too big for me to really be able to review, but for the JNI stuff, it'll be best to only have one library / one interface, rather than two. I would split the crates such that one crate (currently live_update) contains all of the Android/JNI related code, and the other crate (layout?) contains all of the shared code.

iamralpht · 2023-08-31T16:17:14Z

This is too big for me to really be able to review, but for the JNI stuff, it'll be best to only have one library / one interface, rather than two. I would split the crates such that one crate (currently live_update) contains all of the Android/JNI related code, and the other crate (layout?) contains all of the shared code.

Yeah, that makes sense. I think if the library is being used for native targets then developers will come in looking from a native perspective -- given that, I think it would also be good to rename live_update to jni since it contains glue logic and not functional logic relating to the live update feature.

rylin8 · 2023-08-31T18:26:51Z

This is too big for me to really be able to review, but for the JNI stuff, it'll be best to only have one library / one interface, rather than two. I would split the crates such that one crate (currently live_update) contains all of the Android/JNI related code, and the other crate (layout?) contains all of the shared code.

Yeah, that makes sense. I think if the library is being used for native targets then developers will come in looking from a native perspective -- given that, I think it would also be good to rename live_update to jni since it contains glue logic and not functional logic relating to the live update feature.

Done!

iamralpht · 2023-09-04T18:22:12Z

I tried this out a bit on an emulator with 1.0 density.

In general -- initially I got a lot of out-of-memory exceptions, but I didn't try tracking them down. Somewhere we're making a huge allocation.
Alignment: the text isn't aligned correctly; I can take a look at this today, or next weekend.
Battleship: the board is getting stretched in a weird way; I think this might be the parent layout not having the expected values (I definitely saw this design fail like this before in the Rust toolkit).
HConstraints and VConstraints: these aren't stretching out, because I think the incoming Modifier.fillMaxSize() is currently ignored.
Cross Axis Fills looks correct now (and incorrect before)!
Fill Container Test looks correct (but not sure it's getting stretched).
The various interaction and visual (non-layout) validations look fine.
Fancy Fills still seems to crash the emulator (it was before, don't know why yet, no backtrace).

So overall most tests are working. I think making the root stretch/resize to parent by default will either reveal some other layout issues or prove that it's behaving as expected. Text metrics are pretty challenging to get right, and the current implementation is quite complicated because at the time it was written, Compose always included some additional vertical padding on text. Compose changed the text measurement logic in 1.2.0 (hence this confusing "Added in 1.2.0, Deprecated in 1.2.0" note on PlatformTextStyle#includeFontPadding), so we might be able to remove our measurement logic which only existed to remove the extra padding since we only target Compose > 1.2.0.

iamralpht · 2023-09-07T17:36:51Z

@rylin8 on the most recent change for vertical alignment, how does text_size relate to line height? (You can change text size and line height within a single "Text" node, and we put some of that stuff in the text run structure).

rylin8 · 2023-09-07T18:02:50Z

@rylin8 on the most recent change for vertical alignment, how does text_size relate to line height? (You can change text size and line height within a single "Text" node, and we put some of that stuff in the text run structure).

text_size is just the size of the text bounding box in Figma -- it doesn't have anything to do with the line height. The bounding box of a text node in Figma often had a smaller height than the height measured in Compose, so I had to take that into account when calculating layout and where to vertically position the text.

iamralpht · 2023-09-07T18:18:36Z

text_size is just the size of the text bounding box in Figma -- it doesn't have anything to do with the line height. The bounding box of a text node in Figma often had a smaller height than the height measured in Compose, so I had to take that into account when calculating layout and where to vertically position the text.

Got it, yes, that makes sense. Android has really beautiful text layouts, but they tend to be a tiny bit wider than what harfbuzz generates (at least, when we used harfbuzz directly for text layout it seemed to perfectly match what Figma generates itself). I think it could be a glyph offset rounding thing, but I don't know Android's text implementation well enough.

iamralpht · 2023-09-15T06:04:33Z

crates/figma_import/src/transform_flexbox.rs

+                let height_points = bounds.height().ceil();
+                style.width = match frame.layout_sizing_horizontal {
+                    LayoutSizing::Fixed => Dimension::Points(width_points),
+                    LayoutSizing::Fill => Dimension::Points(width_points),


Does it work if you just set min_width / min_height? (You probably tried that...).

Nope that doesn't work, that makes the size grow too large also. I'm thinking I may have to make autolayout frames with children that have a FILL sizing mode to somehow ignore the children when calculating layout size.

Hm, ok, this is another dumb idea, but what are the default settings for flex and align-items? I'm wondering if there's some default style value that's set inappropriately.

flex grow/shrink both default to 0, and align-items defaults to Stretch. However align-items should get set to something else if it is a frame, based on the value of counter_axis_align_items. I can dig it into it more and try and reproduce with a flexbox example somewhere else.

Closes #174, #292 Layout code has been mostly removed from Android. A new layout crate has been created in Rust that handles layout. The general way in which this crate works is: * add_view() adds a view to the view tree * remove_view() removes a view from the view tree * set_node_size() forces a fixed size for a view. This is generally called by text nodes, whose size is calculated in Android * Whenever layout is computed, a list of nodes that have changed is sent to Android In Android, views get assigned a layoutID by a layout manager and subscribe themselves to layout changes for their layoutID. Subscribing calls through to Rust via JNI and adds their view to the view tree using add_view, which is computed by taffy. Whenever taffy does a layout computation, it checks to see which layoutIDs have changed and sends a message back to Android with a list of these IDs. The layout manager then informs all subscribers whose layout changed, and those subscribers recompose. When a view is no longer rendered, it unsubscribes from the LayoutManager, which calls remove_view via JNI. Taffy uses a style structure similar to our toolkit's ViewStyle. To support their structure, a few fields in ViewStyle have been changed or added, and the parsing code in transform_flexbox that takes Figma's node structure and converts it to a ViewStyle has also been modified. ViewStyle gets converted into Taffy's Style structure with the into() trait. Text measurement is still done in Android but has been revamped a bit. New and modified text properties from Figma are now parsed, including trunction and max lines, so there is no need for the text layout plugin. Text is then measured in one of the following ways: 1. Auto width, fixed height: Width should expand as much as needed to fit the text. This always results in a single line of text. 2. Auto height, fixed width: Height should expand as much as needed to fit the text with the text wrapping. If max lines are set, text should stop after that number of lines, ending either with truncation or ellipsis. 3. Fixed size: Width and height are fixed. Text either truncates or elides depending on truncation property. 4. Auto height, fill container width: This special case requires a measure function to get passed into taffy. For this scenario, when the text view adds itself to layout, it also adds a measure function. This measure function is called by taffy possibly several times as taffy computes layout, giving the measure function the text's current and available size. Additionally, since Compose text is a bit different than Figma text, text will look slightly different and be positioned slightly differently in Android than in Figma. To account for this, text has both a layout size/position and a render size/position. The render height and y offset can be slightly different in order to render closely with Figma but still allow for correct layout. Here is a list of additional changes worth noting that were made to support layout in special circumstances: * Component instances: Component instances and the corresponding component set have the same name, so their layout calculation would sometimes overwrite the other one, resulting in the wrong layout for the instance being used. Fix this by ignoring component sets when doing layout. * Component variants: When an instance of a variant changes through an interaction or @DesignVariant customization, in order to avoid its offset from its parent was resetting to 0, provide the layout system with both the new variant and the original variant views so that it can use the offset from original variant. Variant resolution has been moved from DesignFrame to DesignView in order to be consistent with variants changed through an interaction. * Radial gradient crash: Due to layouts from the layout manager possibly being null, don't create a radial gradient if the width is 0 in order to avoid a crash. * To support rotated nodes, add a bounding box property to ViewStyle. Save a node's bounding box, which accounts for scale and rotation transformations on the node, separately from it's size. Use this bounding box as the basis for layout calculations, and continue to use the size property for rendering. * ViewStyle's margin field determines its offset from its parent instead of ViewStyle's left/top fields in order to align with Taffy's style. Several areas of code have been updated to accomodate this change including code support for dials, progress bars, progress markers, and masking. * Since adding a view to the layout manager requires knowing the view's parent ID and its child index into the parent, the customization for replacing children has been modified from a @composable () -> Unit customization to a class called ReplacementContent. Customizing the children of a node now requires a little more work in filling out data for this class such as the number of children, and for each child, the composable for the child. * Several of the battleship tiles were rendering incorrectly after rebasing with main and pulling in the change to support scaled vectors. Support for scaled vectors has temporarily been disabled, as it fixes the rendering of Battleship. Some more investigation is needed to see if we need to do anything to support scaled vectors or if it already works as is, since they seem to look correct now even when stretching the parent to force a vector scale. * In order to reduce recomputing layout over and over again when adding views, don't compute layout until the root view and all its children have finished composition using a boolean in layout manager. This boolean gets reset when we load a new root view. * To support the list preview widget, when encountering a frame that is set up as a widget, the code diverges from the usual means of calculating layout via the Rust layout manager. Instead, children of the list widget are laid out directly with a lazy grid, row, or column, depending on the parameters set in the list widget. Currently there is an issue with the "Hug Contents" feature of horizontal and vertical layouts within the list widget. This should be fixed in a follow up change. * When parsing frames, set size to a fixed size when sizing mode set to FILL. This may not be correct, but without this change, FILL is treated the same as HUG in that the width/height is set to AUTO in ViewStyle, causing FILL containers to grow larger than they should. Need to reassess to see how FILL containers should be sized in taffy. * To support the visibility customization, DesignFrame and DesignText return a boolean indicating whether they were rendered or not. This is important to properly calculate the child index of each visible view for the Rust layout manager -- otherwise it could crash by adding a view with an index higher than the current size of the children list. * Filter out old extended layout properties that may have been written to Figma nodes. These were causing the new layout to think that the list widget was being used when it wasn't. * Added a new layout test activity that tests some custom layout code to see how it works. * To support layout and rendering at different pixel densities, the layout algorithm is done at the raw pixel level (matching Figma). When getting layout values from the layout manager to use in Android, layout values are multiplied by the current pixel density. There are a few scenarios where support for densities other than 1 needed additional changes: - Code that referenced the pixel values specified in ViewStyle, such as margin, width or height, now multiply those values by the pixel density using the pointsAsDp(density) function. This has been fixed in a number of places including dials and gauges and masks, which previously had bugs. - Since text size is calculated in Android and then sent back to the Rust layout system, the size is converted back into raw pixels by dividing by the pixel density first. When calculating text size given existing size data, convert the size data from raw pixels first by multiplying with the pixel density. * To optimize layout, attempt to only compute layout when we are done adding all of a node's children. Add a beginLayout() and endLayout() function to the LayoutManager that keeps track of layout IDs in progress. When a DesignFrame subscribes to layout, call beginLayout() which adds its layout ID to a set of IDs. After the frame's children composables have been called, call endLayout() which removes the layout ID from the set. When the set is empty, the frame and all its children have completed, so compute layout. This reduces layout computation in scenarios such as a LazyGridView with lots of children. This now computes layout for every child of the LazyGridView instead of every ancestor of every child. * Added a unit testing file that should be added to for additional layout features, and for any additional features or bugs we fix. REMAINING ISSUES: - Text alignment with multiple lines is off - Text with constraints not supported - Rotated autolayout frames are not laid out properly - Can't specify Modifiers that change the size or position, such as Modifier.fillMaxSize(), Modifier.offset() - Problems with scale transformations and vector scale - List widget horizontal/vertical layout “Hug Contents” doesn't work - List widget horizontal/vertical layout "space between" doesn't work - Autolayout containers set to FILL don't fill -- they are set to a fixed size. - Don't support scrollable containers that are not in a list widget - Performance issues

@keep

Fixes #406 We can and should use the @keep annotation for any functions and structures that will be called only from the Rust code.

Use the base variant view mechanism to support component replacement node placement. When doing a component replacement, we now pass the parent layout info as context to the replacement node. The parent layout info now has the base view, or original view, which gets sent to the layout manager when subscribing for layout changes. The layout manager uses the original view for positioning the component replacement. Added unit tests for component replacement. Since each unit test adds views to the layout manager, additional functions to clear all views from the layout manager have been added. When running unit tests for layout, tests must be single threaded, so run with: cargo test -- --nocapture --test-threads=1

Apply the size modifier to the Row/Column.

…widgets Don't call frameRender() for list widgets since that clips children and any decorative elements to the widget should be applied to the parent.

Let Row() and Column() determine their own size when hug contents is specified. When the size is determined, let the layout manager know so that it can resize the parent and other nodes if the parent is set to autolayout.

timothyfroehlich · 2023-10-13T21:28:50Z

@rylin8 Are we ready to close this PR since we're working off the new feature branch?

rylin8 · 2023-10-13T21:31:18Z

Merged into feature/rustlayout

rylin8 requested review from kovmarci86, iamralpht, johnoneil and timothyfroehlich August 24, 2023 00:53

rylin8 force-pushed the wb/rylin/shared-code branch from 8fbcd8a to 9a5a81c Compare August 26, 2023 04:43

rylin8 force-pushed the wb/rylin/shared-code branch 3 times, most recently from f432d95 to af70d2c Compare August 30, 2023 21:24

rylin8 force-pushed the wb/rylin/shared-code branch from 6cce407 to 8c9afa0 Compare August 31, 2023 18:26

rylin8 force-pushed the wb/rylin/shared-code branch 2 times, most recently from 543d96c to 0de2c58 Compare August 31, 2023 19:00

rylin8 force-pushed the wb/rylin/shared-code branch from 4ddacd7 to fa0e614 Compare September 7, 2023 18:15

iamralpht reviewed Sep 15, 2023

View reviewed changes

rylin8 force-pushed the wb/rylin/shared-code branch 6 times, most recently from 184f6ba to 64caa2b Compare September 26, 2023 01:09

rylin8 force-pushed the wb/rylin/shared-code branch 4 times, most recently from 0ad55e7 to 217519d Compare September 27, 2023 04:26

timothyfroehlich added the ❗ P1 Priority for next release label Sep 28, 2023

rylin8 mentioned this pull request Sep 28, 2023

Fix rendering of rotated nodes #244

Open

rylin8 force-pushed the wb/rylin/shared-code branch from ecd6165 to 6f357d0 Compare September 29, 2023 06:19

rylin8 mentioned this pull request Sep 29, 2023

Variant resolution with '*' causes a crash #292

Closed

rylin8 force-pushed the wb/rylin/shared-code branch 2 times, most recently from f34ebe5 to 8ba18a1 Compare October 11, 2023 18:10

rylin8 and others added 3 commits October 11, 2023 11:13

Fix: Release builds crash trying to find DesignTextMeasure class (#437)

3e0fd3d

Fixes #406 We can and should use the @keep annotation for any functions and structures that will be called only from the Rust code.

rylin8 force-pushed the wb/rylin/shared-code branch from 8ba18a1 to b80a96f Compare October 11, 2023 18:13

rylin8 added 2 commits October 11, 2023 16:50

Fixes #400: Support "Space Between" in list widget

aa43277

Apply the size modifier to the Row/Column.

Closes #451: Fix cut off items in scrolling horizontal/vertical list …

a6f5874

…widgets Don't call frameRender() for list widgets since that clips children and any decorative elements to the widget should be applied to the parent.

rylin8 force-pushed the wb/rylin/shared-code branch from 799b7ae to a6f5874 Compare October 12, 2023 18:07

Closes #399: Support "Hug Contents" for list widgets.

cbbe380

Let Row() and Column() determine their own size when hug contents is specified. When the size is determined, let the layout manager know so that it can resize the parent and other nodes if the parent is set to autolayout.

rylin8 closed this Oct 13, 2023

rylin8 deleted the wb/rylin/shared-code branch September 25, 2024 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move layout code into Rust #314

Move layout code into Rust #314

rylin8 commented Aug 24, 2023

johnoneil commented Aug 25, 2023

rylin8 commented Aug 25, 2023

iamralpht commented Aug 28, 2023

rylin8 commented Aug 28, 2023

timothyfroehlich commented Aug 31, 2023

iamralpht commented Aug 31, 2023

rylin8 commented Aug 31, 2023

iamralpht commented Sep 4, 2023

iamralpht commented Sep 7, 2023

rylin8 commented Sep 7, 2023

iamralpht commented Sep 7, 2023

iamralpht Sep 15, 2023

rylin8 Sep 19, 2023

iamralpht Sep 20, 2023

rylin8 Sep 20, 2023

timothyfroehlich commented Oct 13, 2023

rylin8 commented Oct 13, 2023

Move layout code into Rust #314

Move layout code into Rust #314

Conversation

rylin8 commented Aug 24, 2023

johnoneil commented Aug 25, 2023

rylin8 commented Aug 25, 2023

iamralpht commented Aug 28, 2023

rylin8 commented Aug 28, 2023

timothyfroehlich commented Aug 31, 2023

iamralpht commented Aug 31, 2023

rylin8 commented Aug 31, 2023

iamralpht commented Sep 4, 2023

iamralpht commented Sep 7, 2023

rylin8 commented Sep 7, 2023

iamralpht commented Sep 7, 2023

iamralpht Sep 15, 2023

Choose a reason for hiding this comment

rylin8 Sep 19, 2023

Choose a reason for hiding this comment

iamralpht Sep 20, 2023

Choose a reason for hiding this comment

rylin8 Sep 20, 2023

Choose a reason for hiding this comment

timothyfroehlich commented Oct 13, 2023

rylin8 commented Oct 13, 2023