search

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

Style sheet lists sometimes cause non-deterministic behavior #50

Closed TheDudeFromCI closed 7 months ago

TheDudeFromCI commented 7 months ago

I have noticed a small bug regarding the current implementation of multiple style sheets and collisions. Despite each layer of the style sheet being written in the intended order, the current cache implementation stores all properties and tracked entities in a hash map based of the asset ID of individual style sheet assets. As assets may have their ID assigned at random, depending on the order they are loaded, this causes the order in which style sheets to be applied to have a random effect with each client restart.

My proposed solution to this would be to change away from the style sheet asset ID behind used as the identifier, and instead using the id of the Entity that contains the StyleSheet component.

https://github.com/afonsolage/bevy_ecss/blob/e32ed2cd4e90308ad3f4482497c592143bd097e6/src/system.rs#L100-L107

https://github.com/afonsolage/bevy_ecss/blob/e32ed2cd4e90308ad3f4482497c592143bd097e6/src/property/mod.rs#L250-L257

Shown above in system.rs line 103, and property/mod.rs line 254, I believe that switching to using an Entity as a map key would be better here, but I'm unsure if this is the best approach. My reasoning is that because all style sheets are stored with a different key, they are not applied at the same time. If an entity has 3 style sheet assets attached to it, all 3 assets should be merged into the same (TrackedEntities, SelectedEntities) and thus all be applied at the exact same time as if they all existed within the same asset.

I would like some feedback before opening a PR.

https://github.com/afonsolage/bevy_ecss/blob/e32ed2cd4e90308ad3f4482497c592143bd097e6/src/property/mod.rs#L325-L336

Here, in property/mod.rs line 333, it's shown how the order of style sheets can be rearranged when being applied to an entity after already being cached. Since this iterator is based off the arbitrary order of Asset IDs instead of the order in which they were written, all style sheets have a separate ID and are effectively applied in a random order.

afonsolage commented 7 months ago

I understood where the problem is. Like you said, the root of the problem is in StyleSheetState since it doesn't track the order of insertion when prepare_state is ran and insert StyleSheetAsset ids.

At this point I think the better approach (or at least the one that I would take) is to rework StyleSheetState to be aware of insertion order.

Since StyleSheetState is precomputed whenever a stylesheet has to be applied, this component can be backed by a Vec. I used HashMap just for convenience. At the top of my head and taking a quick look on the code, I don't see any problems by switching from HashMap to Vec, since the O(n) cost wont matter since usually the StyleSheet count should be fewer than a couple of hundreds at its peak.

TheDudeFromCI commented 7 months ago

@afonsolage Implemented.