Packed Vals#24800
Conversation
| let d = direction * 50.0 * time.delta_secs(); | ||
| let (Val::Px(x), Val::Px(y)) = (transform.translation.x, transform.translation.y) | ||
| let (Val::Px(x), Val::Px(y)) = | ||
| (transform.translation.x(), transform.translation.y()) |
There was a problem hiding this comment.
Given that Val2 is user-facing, making the fields not directly set-able seems like an undesirable UX tradeoff to make in the interest of satisfying what appears to be renderer size requirements?
I think I'd prefer a conversion at the point where that size requirement manifests rather than eating the UX cost everywhere
There was a problem hiding this comment.
It's not about data to the renderer. Responsive coords are completely resolved before we send anything to the renderer, it's just sent physical render coordinates.
What's bothering me, and maybe I'm fussing unnecessarly, is the footprint per UI node entity. Users will want to create complex UIs that feature things like game maps, graphs, inventory screens, etc which can easily have tens of thousands of nodes. So with a huge Node struct we have poorer performance updating and traversing the tree due to poor cache locality. I haven't done much research or benchmarking though, and our layout performance is fine most of time.
With Val2 in particular, I don't think the UX is that much worse (For UiRect it would be much worse, which is why I started with Val2). For construction, types like Val2 using struct literal syntax is quite rare compared to using the builder functions. And for updating values, responsive coords already makes things clumsy, it's not like with Vec2 where you can do my_vec2.x += 10. as the units might not be compatible. So you almost always want to use a helper function or deconstruct for update operations.
My naive idea was to followup this up by deriving FromTemplate for Node and add Val2Template, UiRectTemplate and BorderRadiusTemplate types, like:
pub struct Val2Template {
pub x: Val,
pub y: Val,
}
impl Default for Val2Template {
fn default() -> Self {
Self::from(Val2::default())
}
}
impl From<Val2> for Val2Template {
fn from(value: Val2) -> Self {
Self {
x: value.x(),
y: value.y(),
}
}
}
impl Template for Val2Template {
type Output = Val2;
fn build_template(&self, _context: &mut TemplateContext) -> bevy_ecs::error::Result<Val2> {
Ok(Val2::new(self.x, self.y))
}
fn clone_template(&self) -> Self {
Self {
x: self.x,
y: self.y,
}
}
}Then we can still use the name field syntax and decrease the size. My intuition wrt what's possible with bsn! and what the limitations are isn't that great yet though, maybe it's a bad idea or there are other options.
There was a problem hiding this comment.
I'd want very compelling benchmark results before embracing something like this in the interest of performance. Its not just a UX issue: this introduces more code + complexity to maintain and understand.
|
Do we have any kind of benchmarks that shows this kind of packing is needed? |
Objective
A
Valis 8 bytes, which can't realistically be reduced without using f16s which aren't accurate enough.However with
Val2,UiRectandBorderRadiusyou can combine theValenum discriminators into one value. The reducesVal2from 16 bytes down to 12, andUiRect/BorderRadiusgoes from 32 bytes down to 20 bytes. IfBorderRadiusis extended to support elliptical radii (#24779) it would need 64 bytes which could be packed down to 36 bytes.Solution
packandunpackfunctions on Val, map it to and from a (u8, f32) pair.Val2to use packedVals.The discriminator might be better as a #[repr(u8)] enum with explicit values, though then you'd have to do a match or transmute to unpack.
It might not a be a very popular change, I'm not quite sure how well it works API wise. Maybe something could be done with bsn! though.