afonsolage / bevy_ecss

Bevy crate which uses a subset of CSS to update Bevy ECS components
Apache License 2.0
98 stars 11 forks source link

Consider switching from `cssparser` to `lightningcss` #33

Open afonsolage opened 9 months ago

afonsolage commented 9 months ago

When upgrading from 0.29 to 0.33 on #32, the lack of migration guide and changelog of cssparser crate imposed some challenges which we could avoid completely by switching to https://github.com/parcel-bundler/lightningcss.

I don't know if we could keep the same API, but this should be possible, since lightningcss is a full fledged css parser and also support custom properties (which is a hard requirement for our crate).

Leinnan commented 9 months ago

If you think that could lead to smaller codebase and more functionality, I am all for it. Maybe even it would make easier handling #18 and #14

afonsolage commented 9 months ago

Yes, it should all be there already, since lightningcss is used by Parcel

afonsolage commented 9 months ago

Yes, using lightningcss seems doable. I was taking a look and the big change is that we must divide the crate in two pieces (as planned in #28)

Official CSS support

Everything is already there on lightningcss, all possible selectors, properties and rules, we must have to decide which one of these makes sense in the bevy_ui context. They even already have PseudoClass.

A small problem is that the docs.rs version is outdated and we must download the source and build the docs ourselves, but I'll link here the relevant enums or sources which already contains all variants we would ever need:

So instead of creating our own properties and such, we just map to implement existing ones from lightningcss. If a property isn't implemented/supported, this should be a warn!.

Bevy ECS support

lightningcss also allows us to implement custom behaviour to interact with bevy_ecs by using either custom variants when available or parsing unknown ones.

We could also experiment with expanding bevy_ecs support, but I think we should avoid expanding beyond "styling", since bsn! is around the corner and there also proper scene formats to spawn and work with scenes.

Closing thoughts

Moving to lightningcss will be a big win, since we shouldn't worry about parsing css files and lightningcss is battle tested, so we could focus on Bevy context, which is what we want.

We should create more and better examples and tests, to ensure everything will be fine after the migration, which should be a major release.

Leinnan commented 9 months ago

Agreed on all of it 😅 I think even that #30 could be done as part of that task.

I was also thinking about expanding the library to have some kind of support for reading UI from file, but that aspect is going to change soon so that something that is not worth the effort, at least now.

afonsolage commented 9 months ago

I was also thinking about expanding the library to have some kind of support for reading UI from file, but that aspect is going to change soon so that something that is not worth the effort, at least now.

I don't think worth it we expanding bevy_ecss beyond "styling", even if we decided to "style" Transforms and others non-css standards components, the main idea still holds, which is to have a quick and well-known way of having state saved for bevy_ecs components.

Even when bsn! lands, which we should see it's first MVP in the next weeks, bevy_ecss will still be relevant, since many folks already know how css works, so they probably will create the scene using bsn!, but will debug/tweak/apply themes using css which is a more widely known way of doing so.

TL;DR; we should keep bevy_ecss as an abstraction layer over bevy_ui, bevy_ecs and in the future, bsn!, so people can easily poke those things using a known format, which is css.

afonsolage commented 9 months ago

I've done a deep dive into lightningcss and one main problem that I've found is since all Propertyes are variants of an enum, we can't use a single variant as a associated type on Property trait, so the user API will be more complex than is today.

Until now the only gain that I'm seeing is using selector instead of using our own, but we don't need the whole lightningcss for that.

Tomorrow I'll keep looking and check if there is a way to use the types in lightningcss for our advantage.

afonsolage commented 9 months ago

The entire lightningcss crate was built to work with borrowed values and requires lifetime for all structs. Unfortunately this doesn't work well with out caching strategy, which parses the css file into a RuleList, so we doesn't have to parse css file everytime we need to apply a style.

I'll keep this issue open, in case we figure out somehow a way to have an "owned" StyleSheet from lightningcss. Without this, I can't see a way to use it.

I've pushed the branch https://github.com/afonsolage/bevy_ecss/tree/lightningcss which I was working on, but there is little work done (only usage of PropertyId).

Leinnan commented 9 months ago

I think that right now would make more sense to focus on trying out selector.

afonsolage commented 9 months ago

I've dove deep into selectors today and the biggest problem is that all those crates are created to avoid memory copying, which is a hard requirement in the browser, to reduce memory footprint. In short, given a css loaded into memory, all those crates assumes a reference to this file will always be available and everything can be done without memory copying.

But in bevy_ecss things doesn't work like that, since we cache the Selectors and Properties, so we may run more than once every frame and also due to parallelism (every property have its own system), we always have to keep Properties and Selectors in memory, detached from original css asset. To be precise, after css asset file is loaded, it's content is dropped, since we keep an internal list of parsed StyleRules.

In the end it was a good exercise to understand why our implementation is different, and since cssparser gives already everything we need, it doesn't seems we could use selectors crate.

We would have to implement all the logic anyways, the gain was just to be able to skip the StyleSheetParser, which isn't that complicated.

afonsolage commented 7 months ago

Take a look on this discussion on how to implement selectors: https://discord.com/channels/691052431525675048/1186021848018854049

Leinnan commented 7 months ago

Okay- it looks like a good starting point but being honest I do not know how much time it would take to update this crate to use selectors for matching. Because based on theirs comments, it seems pretty simple to do.