DioxusLabs / taffy

A high performance rust-powered UI layout library
https://docs.rs/taffy
Other
2.06k stars 102 forks source link

Consider not storing properties of disjoint layout options in the main Style struct #605

Open stephanemagnenat opened 7 months ago

stephanemagnenat commented 7 months ago

What problem does this solve or what need does it fill?

At the moment, the Style struct contains both properties common to all layout options (e.g., margin), but also specific ones to Flexbox, Grid, etc. This feels wrong to me, as it consumes extra memory that is not needed (if I'm not mistaken, the Flexbox properties do not make sense when using a Grid layout).

What solution would you like?

The layout-specific properties could be stored inside inner structs specific to each variant of the Display enum. This would possibly allow to keep each of these inner structs in their own modules, reducing the amount of #[cfg(feature = "...")] needed inside the style top module.

What alternative(s) have you considered?

The current approach can be kept, it doesn't hurt, it just uses a bit of extra memory and makes the Style struct quite big.

Additional context

I am considering using taffy for my own GUI framework, to be open sourced one day if/when it exists enough.

Patryk27 commented 7 months ago

Note that a (huge, in my opinion) downside of using an enum would be that changing the display mode would remove current values from the memory.

That is, patterns such as (in pseudo-code):

fn initialize(ui: &mut Ui) {
    widget.display = Display::Flex;
    widget.align_self = AlignSelf::FlexEnd;
    widget.flex_direction = FlexDirection::RowReverse;
}

fn update(ui: &mut Ui) {
    if something {
        widget.display = Display::Flex;
    } else {
        widget.display = Display::None;
    }
}

... would stop working, as user would need to re-do the initialization once again during the update; that's not the worst thing in the world, of course, but it's very handy.

There's also the issue of convenience, as having to do:

// init:
widget.display = Display::Flex(Flex {
    flex_direction: FlexDirection::Something,
    ..Default::default()
});

// update:
widget.as_flex().unwrap().flex_direction = FlexDirection::Something;

... can get awkward.

stephanemagnenat commented 7 months ago

That is true that these use cases would get harder.

However, I am wondering whether this first pattern is not mostly a side effect of the web/CSS habits rather than good programming practice. Because in principle, if there is a specific FlexInner configuration to use in a given context, I would expect that this specific configuration could just be a constant that is re-used (assuming FlexInner is Copy):

const FLEX_CONFIG: FlexInner = FlexInner {
     align_self: AlignSelf::FlexEnd,
     flex_direction = FlexDirection::RowReverse,
    ...
};

fn initialize(ui: &mut Ui) {
    widget.display = Display::Flex(FLEX_CONFIG);
}

fn update(ui: &mut Ui) {
    if something {
        widget.display = Display::Flex(FLEX_CONFIG);
    } else {
        widget.display = Display::None;
    }
}

Alternatively, there could be a function that returns the specific flex display config:

fn display_flex(): Display {
    Display::Flex(FlexInner {
        align_self: AlignSelf::FlexEnd,
        flex_direction = FlexDirection::RowReverse,
        ...
    })
}

fn initialize(ui: &mut Ui) {
    widget.display = display_flex();
}

fn update(ui: &mut Ui) {
    if something {
        widget.display = display_flex();
    } else {
        widget.display = Display::None;
    }
}

This function could take some parameters if needed.

So, for the second example, yes in that case the as_flex().unwrap() would be needed, but if we imagine a function returning the display setting given a specific parameter, then it is not really a problem. The drawback would be some more code execution, but in the code-vs-memory tradeoff, in all generality I would tend favor memory nowadays.

nicoburns commented 7 months ago

There's a bit of a tradeoff between memory usage and layout performance here (as any compressed representation will need to be translated when performing layout (which is currently made more significant by the fact the algorithm code will request access to the style multiple times during the layout of each node).

I think the plan is something like https://github.com/DioxusLabs/taffy/pull/568 which will allow consumers of Taffy to bring their own style representation (although the details of the implementation may well change). That could be: