Unify encoding of Scene/SceneFragment #426
Conversation
In a move toward dropping the SceneFragment and SceneBuilder types, this unifies the underlying encoding of Scene and SceneFragment. Specifically, we no longer encode an initial transform and style for Scene. This causes the transform and style indices in the path tag monoid to be off by one. I chose the simple approach of subtracting 1 from each in the combine_tag_monoid() functions in both GPU and CPU code to correct this.
This removes both `SceneBuilder` and `SceneFragment` while moving all of the "builder" methods directly to `Scene`. Based on #426 which must land first.
There was a problem hiding this comment.
Yes, this does seem reasonable.
If we wanted to strictly optimize for shader speed, it would be possible to remove the subtract operations by tweaking config.style_base and config.transform_base, but I think that's at the cost of code clarity (it makes the invariant less clear).
If we were documenting a semi-public interface for encoding, we'd also want to explicitly state that the encoded scene must start with a style and transform before use. We're not committing to that now, but if you feel like adding a comment, it wouldn't hurt. On a quick look I think the rustdoc for Encoding would be a good place.
I've verified that current code paths ensure valid style and transform, including the main path encoding and glyph resolution. Obviously if we add more, we'll need to keep this in mind, and we may come back and optimize this further, trimming redundant transforms/styles if we encounter them. But that latter is a low priority.
Thanks!
This removes both `SceneBuilder` and `SceneFragment` while moving all of the "builder" methods directly to `Scene`. Based on #426 which must land first.
In a move toward dropping the SceneFragment and SceneBuilder types, this unifies the underlying encoding of Scene and SceneFragment. Specifically, we no longer encode an initial transform and style for Scene.
This causes the transform and style indices in the path tag monoid to be off by one. I chose the simple approach of subtracting 1 from each in the combine_tag_monoid() functions in both GPU and CPU code to correct this.
@raphlinus does this seem reasonable to you?