Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
stephanemagnenat opened this issue Jan 25, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@stephanemagnenat
Copy link

stephanemagnenat commented Jan 25, 2024

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.

@stephanemagnenat stephanemagnenat added the enhancement New feature or request label Jan 25, 2024
@stephanemagnenat stephanemagnenat changed the title Consider not having style for disjoint layout options in the same struct Consider not having properties for disjoint layout options in the main Style struct Jan 25, 2024
@stephanemagnenat stephanemagnenat changed the title Consider not having properties for disjoint layout options in the main Style struct Consider not storing properties of disjoint layout options in the main Style struct Jan 25, 2024
@Patryk27
Copy link

Patryk27 commented Jan 27, 2024

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
Copy link
Author

stephanemagnenat commented Jan 28, 2024

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
Copy link
Collaborator

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 #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:

  • A great big struct like Taffy currently uses
  • A smaller struct supporting only a subset of the properties
  • Something like a Vec<Property>
  • Style properties that are Boxed/Arc'ed/Arena allocated in some way
  • etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants