viridia / quill_v1

Reactive UI framework for Bevy game engine
MIT License
60 stars 7 forks source link

Excessive cloning of props and local state accessors. #15

Closed viridia closed 9 months ago

viridia commented 9 months ago

I'm not sure that this problem can be solved, but it sure is painful. When you have a component that has one or more closures, you have to clone every captured prop or local state accessor:

// Horizontal slider widget
pub fn h_slider(mut cx: Cx<SliderProps>) -> impl View {
    let drag_offset = cx.use_local::<f32>(|| 0.);
    let is_dragging = cx.use_local::<bool>(|| false);
    // Pain point: Need to capture all props for closures.
    let id = cx.props.id;
    Element::new()
        .styled(STYLE_SLIDER.clone())
        .with(move |entity, world| {
            let mut e = world.entity_mut(entity);
            let mut drag_offset_1 = drag_offset.clone();
            let drag_offset_2 = drag_offset.clone();
            // *Horrible*: we need to clone the state reference 3 times because 3 event handlers.
            let mut is_dragging_1 = is_dragging.clone();
            let mut is_dragging_2 = is_dragging.clone();
            let is_dragging_3 = is_dragging.clone();
            e.insert((
                On::<Pointer<DragStart>>::run(
                    move |ev: Res<ListenerInput<Pointer<DragStart>>>,
                          mut query: Query<&mut ElementClasses>| {
                        // Save initial value to use as drag offset.
                        drag_offset_1.set(value);
                        is_dragging_1.set(true);
                    },
                ),
                On::<Pointer<DragEnd>>::listener_component_mut::<ElementClasses>(
                    move |_, classes| {
                        is_dragging_2.set(false);
                    },
                ),
                On::<Pointer<Drag>>::run(
                    move |ev: Res<ListenerInput<Pointer<Drag>>>,
                          query: Query<(&Node, &GlobalTransform)>,
                          mut writer: EventWriter<OnChange<f32>>| {
                        if is_dragging_3.get() {
                           // process event
                        }
                    },
                ),
            ));
        })
}

The problem arises because neither props or state accessors implement Copy, and therefore the only alternative is marking the closures as Move which requires a distinct instance for each closure. So if you have N closures you need N clones.

BTW, the reason that local state accessors don't implement Copy is that they contain an Arc, which can only be cloned. Props, on the other hand, can either be copied or not depending on the type, but the lifetime of Props isn't long enough (it's long-lived; a copy of Props is cached in the PresenterState - but may not be as long as the event handlers.)

This seems like a fundamental limitation of Rust closures. The question is whether we can come up with a better developer experience - I don't have any good ideas at this point.

viridia commented 9 months ago

So the solution here, I think, is that we need to make it so that event handlers have lifetimes which are bound by the PresenterState - that is, the event handler lives no longer than the view that created it. Next time we do a data update, the views are recreated, along with the event handlers attached to them.

This would mean that closure captures would be guaranteed to outlive the event handlers (I think).

viridia commented 9 months ago

Fixed, removed use_local, now uses 'atoms'.