SVG decorators, caching and content cropping #598
Conversation
There was a problem hiding this comment.
Thanks a lot for this PR, and sorry for the long delay. I have taken a detailed look at this one now. I think this functionality is great, and the SVG decorator is something that has been requested by multiple users for some time. The caching behavior too is very much welcome.
A note about formatting. I see that these files are checked in with \r\n line endings. This is a bit problematic, especially on existing files as it ruins the git diff. I suggest that you use git config core.autocrlf true for this repository, and check in all the files again. Additionally, the C++ code has been formatted in a different convention, it would be very helpful if you could apply our clang-format file to all the formatting.
I like the overall architecture and separation of concerns here!
One aspect I find somewhat error-prone is the manual reference counting in the cache. It is easy to get this wrong when making changes in the future. I suggest that we replace them with shared pointers, and ensure that their destructors take care of cleaning up.
There are quite a few breaking changes after the effects branch was merged into master. This might require some changes to make it compile and work again after rebasing the PR. Textures and geometry in particular are constructed a bit differently, please take a look at the changelog. Let me know if you have any questions or want me to help make the necessary changes.
Can you elaborate a bit on the SVG caching policy? Looks like the SVGs are cleaned up immediately after all references to them are gone. Are there different policies for the SVG documents and e.g. dimensioned textures? I wonder if we might want to make the same behavior as for file textures. These are always retained until explicitly released, or the library is shutdown. Although only SVG documents would be retained, not the individual dimensioned textures, as those could quickly grow large with e.g. resizing.
Finally, it would be great if we could add a decorator to the SVG sample.
Let me know if you have any questions. Looking forward to following up on this and seeing it merged.
| SVGHandle handle = 0u; | ||
| Utilities::HashCombine(handle, source); | ||
| Utilities::HashCombine(handle, dimensions.x); | ||
| Utilities::HashCombine(handle, dimensions.y); | ||
| Utilities::HashCombine(handle, content_fit); | ||
| Utilities::HashCombine(handle, *reinterpret_cast<uint16_t const*>(&colour)); |
There was a problem hiding this comment.
I think generally it is best to avoid using hash values as unique lookup keys, due to the (rare) case of hash collisions. Instead, one can make a struct holding all of these fields (and provide this hash function for it), or make a unique string value as a key.
|
|
||
| // The image's intrinsic dimensions based on the svg data. | ||
| Vector2f intrinsic_dimensions; | ||
| UniquePtr<lunasvg::Document> svg_document; |
There was a problem hiding this comment.
The main reasons this was wrapped in a unique pointer previously was (1) to be able to forward declare it in the header, and (2) to make it easily resettable. None of these reasons apply anymore so I think we can drop the unique pointer. The same may apply to texture above, and possibly geometry unless we need a stable pointer.
| if (source[source.size() - 1u] == ',') // there seems to be an issue where the decorator csvs include the comma | ||
| source = source.substr(0, source.size() - 1u); |
There was a problem hiding this comment.
The way the shorthand is defined, there should not be a comma between the source value and the keyword here, only a simple space. That is probably why the comma was picked up by the source value. With that in mind, we should be able to remove these two lines.
| class SVGCache | ||
| { | ||
| public: | ||
| static void Deninitialize(); |
There was a problem hiding this comment.
We commonly use the term "Shutdown" throughout the library for this (there is also a spelling error here).
|
|
||
| if (changed_attributes.count("crop-to-content")) | ||
| { | ||
| content_fit = GetAttribute<bool>("crop-to-content", false); |
There was a problem hiding this comment.
In RML and HTML alike, the presence of an attribute is normally used rather than a boolean value. Thus, I suggest that we replace this with HasAttribute.
| @author Leah Lindner | ||
| */ | ||
|
|
||
| class DecoratorSVGInstancer : public DecoratorInstancer |
There was a problem hiding this comment.
This is a minor thing, but lately we have moved these instancer classes into the same file as the decorator itself. It would be nice to follow this recent convention.
|
|
||
| SharedPtr<Decorator> DecoratorSVGInstancer::InstanceDecorator(const String&, const PropertyDictionary& properties, const DecoratorInstancerInterface&) | ||
| { | ||
| String source = properties.GetProperty(source_id)->Get<String>(); |
There was a problem hiding this comment.
Looks like the decorator does not work with relative paths from the document. We can replicate how the document path is joined as in the SVG element. You might need to add a function to DecoratorInstancerInterface to access the document path.
| DecoratorSVGInstancer::DecoratorSVGInstancer() | ||
| { | ||
| source_id = RegisterProperty("source", "").AddParser("string").GetId(); | ||
| crop_id = RegisterProperty("cropping", "none").AddParser("keyword", "none, content").GetId(); |
There was a problem hiding this comment.
The content keyword should match the RML attribute. I remember we discussed the name but don't remember if we agreed on something, I think crop-to-content is fine, or perhaps just crop-content.
The none content is a bit problematic, since it might not be distinct if we add other properties here later. In particular, in the future we might want to add the scaling modes and alignment from the image decorator, and maybe explicit sizing. none could mean several things then, so I suggest crop-none.
|
|
||
| GeometryUtilities::GenerateQuad(&vertices[0], &indices[0], Vector2f(0, 0), render_dimensions, colour, texcoords[0], texcoords[1]); | ||
|
|
||
| coloured_geo.geometry->SetTexture(tex_size.texture.get()); |
There was a problem hiding this comment.
Looks like this is missing some of our recent changes, in particular the compatibility to the color channel changes in LunaSVG v2.3.2.
It also doesn't compile right now, partly due to some callback signature changes here. In any case, some further changes will be necessary when rebasing on master.
Hi, so the main change here is that with these changes the SVG plugin also adds SVG decorators, so from rcss we can set a property like so:
decorator: svg(my_svg.svg);This is particularly useful for things such as using svgs to decorate input elements like checkboxes, which would have previously required making custom elements instead, but I'm sure there will be many other possible use cases.
To make this practical, I also added an optimization where the baked SVG textures are cached so that if more than one element or decorator uses the same SVG with the same size, we don't create multiple instances of the same texture. It even caches the geometties by associated image colour.
Finally, a neat little feature that is added by this is the ability to set the SVG renderer to crop an SVGs viewbox to its content (so if an svg images content doesn't fill up the entire viewbox, it is scaled up so that the content reaches each edge of the resulting texture). This can be set on elements by setting attribute
crop-to-content="true"or setting thecroppingproperty on the SVG decorator tocontent(or usingcontentas the second parameter in the svg decorator shorthand)