compiler: support dynamic z-ordering#11626
Conversation
|
|
|
(Note: this is my agent) |
| pub dynamic_z_child_refs: Vec<NamedReference>, | ||
| /// Compile-time z values for repeater/conditional children (child_index, z_value). | ||
| /// These children can't have their z materialized on the parent component. | ||
| pub dynamic_z_child_constants: Vec<(usize, f32)>, | ||
|
|
There was a problem hiding this comment.
I'm thinking this should be within the children element itself. like the geometry. There could be a z_order: Option<enum { Constant(f32), Dynamic(NamedReferece) }>, so it is easier to track them when the children vector changes. due to other passes
There was a problem hiding this comment.
Thanks! Moved to a per-child z_order: Option<ZOrder> enum field (Constant(f32) / Dynamic(NamedReference)). This way children can be reordered by other passes without breaking anything.
| } | ||
|
|
| let comp_rc = instance_ref.self_weak().get().unwrap().upgrade().unwrap(); | ||
|
|
||
| // Macro-like helper to avoid duplicating the visit_dynamic closure | ||
| macro_rules! visit_dyn { |
There was a problem hiding this comment.
Why can't this be a helper lambda instead of a macro?
There was a problem hiding this comment.
It can't be a regular closure due to the generativity lifetime on Instance<'_> — visit_item_tree_with_sorted_children requires the closure to work for any lifetime (it's monomorphized over the Base type), and a captured closure gets pinned to a specific lifetime. Added a comment explaining this.
| .unwrap_or(0.0), | ||
| }) | ||
| .collect(); | ||
| let sorted = i_slint_core::item_tree::compute_sorted_children_by_z(&z_values); |
There was a problem hiding this comment.
Would it not be good to cache this in a property similar to the layout cache?
There was a problem hiding this comment.
Good idea. I think caching (similar to layout_cache) would be a good follow-up optimization — it would avoid recomputing on every visit when z values haven't changed. For now this recomputes each time, which keeps the initial implementation simple and correct. Happy to add caching in a follow-up if you'd prefer it in this PR though.
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn slint_compute_sorted_children_by_z( | ||
| z_values: Slice<f32>, | ||
| out: *mut crate::SharedVector<f32>, |
There was a problem hiding this comment.
Can that be a &mut SharedVactor then there is less unsafe. Also more correct since the current code overwrite the previous SharedVector which doesn't seems alright.
There was a problem hiding this comment.
Done. Changed to &mut SharedVector<u32> — no more unsafe pointer write.
| /// Returns a `SharedVector<f32>` where each entry is a child offset (relative to children_index) | ||
| /// sorted by the corresponding z value (stable sort). | ||
| pub fn compute_sorted_children_by_z(z_values: &[f32]) -> crate::SharedVector<f32> { | ||
| let mut indices: alloc::vec::Vec<u32> = (0..z_values.len() as u32).collect(); |
There was a problem hiding this comment.
This intermediate vector means we do twice the allocations.
Can we work directly on the final crate::SharedVector<f32> ?
There was a problem hiding this comment.
Fixed. Now works directly on SharedVector<u32> — uses resize + make_mut_slice to initialize and sort in place.
| /// Compute a sorted list of child indices based on their z values. | ||
| /// Returns a `SharedVector<f32>` where each entry is a child offset (relative to children_index) | ||
| /// sorted by the corresponding z value (stable sort). | ||
| pub fn compute_sorted_children_by_z(z_values: &[f32]) -> crate::SharedVector<f32> { |
There was a problem hiding this comment.
What's the reason we don't use something like crate::SharedVector<u32> or crate::SharedVector<usize> (since these are indices)?
i.e: why do we use f32 and not some integer type for the indices?
There was a problem hiding this comment.
Good point. Changed to SharedVector<u32> throughout (FFI, codegen, interpreter).
| /// Like `visit_item_tree`, but uses a pre-computed sorted order for children instead of | ||
| /// the sequential order from the item tree array. | ||
| /// `sorted_children_offsets` contains child offsets (0-based relative to children_index) | ||
| /// in the desired visitation order (back-to-front). | ||
| pub fn visit_item_tree_with_sorted_children<Base>( | ||
| base: Pin<&Base>, |
There was a problem hiding this comment.
To avoid code duplication, could we merge visit_item_tree_with_sorted_children and visit_item_tree into one, (by having the sorted_children_offsets be Option<>
Would that Help?
There was a problem hiding this comment.
Done. Merged into a single visit_item_tree with sorted_children_offsets: Option<&[u32]>.
| TraversalOrder::BackToFront => i, | ||
| TraversalOrder::FrontToBack => count - 1 - i, | ||
| }; | ||
| let idx = *children_index + sorted[offset_idx]; |
There was a problem hiding this comment.
If you write this as let idx = sorted_children_offsets.map_or(offset_idx, |sorted| sorted[offset_idx]) you can greatly simplify the two branches. (also count and children_count should be the same, you can debug_assert that.)
There was a problem hiding this comment.
Done. Simplified using map_or and added the debug_assert_eq.
| /// If set, this node's children have dynamic z-ordering. | ||
| /// Each entry is (child_offset, source_of_z_value). | ||
| /// The code generator will read these z values at runtime and sort children accordingly. | ||
| pub z_sort_order_property: Option<Vec<(u32, ZChildSource)>>, |
There was a problem hiding this comment.
Is the first u32 always the index within the vector?
There was a problem hiding this comment.
Yes, it was always the sequential index (0, 1, 2, ...). Removed it — now Vec<ZChildSource> instead of Vec<(u32, ZChildSource)>.
| for p in public_properties { | ||
| visit_public_property(p, &scope, state, visitor); | ||
| } | ||
| visit_tree_node_z_properties(&mut item_tree.tree, &scope, state, visitor); |
There was a problem hiding this comment.
I think we miss some trees. Like what about popup and stuff?
There was a problem hiding this comment.
Good catch. Added z-property visits for popup windows and popup_menu trees in both count_property_use and remove_unused passes.
Allow the `z` property to accept dynamic expressions (e.g., `z: is_selected ? 10 : 0`) so element stacking order can change at runtime. Previously z was evaluated at compile time and removed. The implementation follows the geometry property materialization pattern: z bindings are kept on children, `materialize_fake_properties` creates declared Property<f32> fields, and `move_declarations` moves them to the component root. At runtime the code generator reads z values, sorts child indices, and traverses children in sorted order. Repeater and conditional children (for/if) participate with their z value evaluated at compile time when constant, or defaulting to 0 otherwise. All three backends are supported: Rust code generator, C++ code generator (via new FFI functions), and interpreter (via z_order_info on ItemTreeDescription). Closes: slint-ui#221
Per-item dynamic z inside repeaters (e.g. `z: model_value`) is not yet supported. Emit a clear compile error instead of silently defaulting to z=0. Also update the screenshot test to avoid using dynamic z in repeaters.
Move z-order info from parent vectors to per-child `z_order: Option<ZOrder>` field, making it resilient to children reordering by other passes. Use `} else if` style. Add comment explaining why macro is needed in interpreter (generativity lifetime prevents using a regular closure).
- Remove unused `root` parameter from `visit_tree_z_properties` (clippy::only_used_in_recursion) - Collapse nested `if` into `let` chain in interpreter (clippy::collapsible_if) - Escape `SharedVector<f32>` in doc comment (rustdoc unclosed HTML tag)
SharedVector has a non-trivial destructor in C++, causing the C++ ABI to expect the return value via sret (hidden pointer), while Rust returns the pointer-sized repr(C) struct in a register. Use an out-pointer parameter instead, matching the pattern used by other FFI functions.
- Use `u32` indices instead of `f32` for sorted child offsets - Work directly on `SharedVector<u32>` to avoid double allocation - Merge `visit_item_tree_with_sorted_children` into `visit_item_tree` with an `Option<&[u32]>` parameter to reduce code duplication - Use `&mut SharedVector<u32>` in FFI instead of raw pointer for safety - Add missing skia screenshot reference for zorder_dynamic test
- Extract `has_z_binding` / `get_z_expr` helpers in z_order.rs to reduce redundant borrows in the first-pass classification loop - Replace silent `.min()` truncation with `debug_assert_eq` in visit_item_tree sorted path (mismatched count is always a bug) - Remove unnecessary "what" comments
- Simplify visit_item_tree children loop using map_or to merge the sorted/unsorted branches into one - Remove redundant child index from z_sort_order_property, since entries are always sequential (Vec<ZChildSource> instead of Vec<(u32, ZChildSource)>) - Visit z properties in popup windows and popup_menu trees in both count_property_use and remove_unused passes - Fix clippy collapsible_if warnings in z_order pass
6c4becf to
f40ea36
Compare
Summary
Allow the
zproperty to accept dynamic expressions (e.g.,z: is_selected ? 10 : 0) instead of only compile-time constant literals. Static z values continue to use the existing zero-cost compile-time reordering.Property<f32>fields (same pattern as x/y/width/height geometry properties)ZChildSourceenumvisit_item_tree_with_sorted_childrenstable-sorts children by z value during traversalCloses #221
Test plan
tests/cases/children/zorder_dynamic.slint— click tests with property-bound z, multi-layer selection, and mixed static/dynamic (Rust, C++, JS)tests/screenshots/cases/basic/zorder_dynamic.slint— visual test with init-assigned z and if-conditionalzorder.slint) continues to pass