alexheretic / glyph-brush

Fast GPU cached text rendering
Apache License 2.0
673 stars 52 forks source link

More flexible `X`/"extra" type handling #163

Closed alexheretic closed 1 year ago

alexheretic commented 1 year ago

This is a less breaking version of #162. Mainly rolling back the Section::default changes and adding Section::builder to help when using a custom X. This avoids the breakage that the Section::default change would have caused.

It's still a little breaking with the Text::new changes. However, I'm willing to try releasing that as a non-breaking bump as I suspect this won't break usage in the wild.

Closes #162

alexheretic commented 1 year ago

@aweinstock314 with this version I'd be comfortable releasing as a non-breaking bump, does that work for you?

I'm not against breaking changes, they'll just take a little more thought as I want to minimise the number of different breaking versions so want to bundle up some other breakers too.

aweinstock314 commented 1 year ago

Yes, this version suffices to make https://github.com/hecrj/wgpu_glyph/pull/96 work (Section::new already doesn't constrain X=Extra, so I'm using that for the examples there, though that becomes a breaking change for wgpu_glyph).

Should I leave #162 open, as the breaking changes version, or close it?

alexheretic commented 1 year ago

Thanks @aweinstock314 I opened #164 to help remember to fix the Section::default() thing.

I wonder if there's a different & cleaner way to do the X stuff, like type-erasure. Hopefully I'll find some time to look into it. Overall I'm not 100% happy with the complexity of the generics. I've tried to be "easy to use" directly but at the same time customisable for wrapper libraries like _wgpuglyph, but it's definitely not perfect.

aweinstock314 commented 1 year ago

It's reasonably straightforward to extend in a single-inheritance way with the current approach, via adding the parent struct as a field and implementing Deref/DerefMut into the parent (along with Debug/Clone/Copy/Hash/PartialEq/Default). It's a bit verbose when adding an extension trait to provide the with_field methods for all the inherited fields (each struct has to mention all of its ancestor's fields) ~(though this might not be a true limitation, and deref coercion might just do the right thing)~.

Making use of deref coercion doesn't support multiple inheritance though (e.g. with the classic diamond inheritance example where ExtraB and ExtraC each extend ExtraA, and ExtraD wants to extend ExtraB and ExtraC, ExtraD can't have multiple Deref impls targetting both ExtraB and ExtraC).

HashMap<String, Box<dyn Any>> might work as a dynamic expansion mechanism, but it'd be less statically checked and probably less performant (i.e. there would be runtime checks for fields not existing and fields being the wrong type).

frunk's LabelledGeneric might be worth looking into. Essentially each Extra struct would contain only the fields it needs and derive LabelledGeneric, and the overall type to use as the extra data would be the hlist-concatenation of each struct's LabelledGeneric::Repr. Each struct-definer would define with_field methods for any hlist containing their fields (so each updater would only need to be defined once). Any struct containing any subset of the fields could be materialized from the combined representation with LabelledGeneric::from. It doesn't look like frunk::hlist has concatenation yet, but it does have foldr, so it could probably be straightforwardly expressed as concat xs ys = foldr cons ys xs at the trait level. This would be entirely statically checked, but would be more heavyweight trait-wise than the current approach.

aweinstock314 commented 1 year ago

Update:

Removing with_color and with_z from TextExt and adding a DerefMut<glyph_brush::Extra> for wgpu_glyph::Extra impl doesn't help since the with_field methods take self, not &mut self, but it wouldn't be ergonomic to change that, since (among other things), that would mean that you'd need to set child fields before parent fields, since each with_field method would return a reference to the deref target. This means that the limitation from the first paragraph is a true limitation: with_field methods need to be repeated for each ancestor field in each child, even in the single-inheritance case.