Basic Inline SVG Implementation #591
Conversation
JailbreakPapa
changed the title
There was a problem hiding this comment.
Hey, and thanks for the pull request! I'm always happy to see new contributors. And I like the feature, with the ability to load inline svg. That could be quite handy especially for some smaller vector graphics. I have some comments here, please let me know if anything is unclear or I can help out in any way.
One thing I notice is that all the descendants of <svg> are added as normal RML elements, thus, taking part of our DOM with normal layout and rendering procedures. Having it be part of the DOM would be cool if manipulating the nodes from RmlUi would actually affect it, like setting their color properties and so on. But we don't support that here. And each node takes up resources that are not relevant to the SVG nodes.
That's why I wonder if it would perhaps be better to hide the inner nodes from the DOM, and simply consider the inner nodes as textual SVG data - the same way that CSS is simply treated as data of a <style> node. What do you think about this?
| </style> | ||
| </head> | ||
| <body template="window"> | ||
| <label>Inline SVG Loaded from RML</label> | ||
| <svg width="800px" height="800px" viewBox="0 0 32 32" fill="none" xmlns="http://www.w3.org/2000/svg"> |
There was a problem hiding this comment.
We should add a source and a license for this SVG file. Please see the License.txt file in the same directory, in addition to an inline comment here.
| </style> | ||
| </head> | ||
| <body template="window"> | ||
| <label>Inline SVG Loaded from RML</label> |
There was a problem hiding this comment.
- Labels are more commonly used for form controls, I think it makes more sense to use headers, such as
h3. - I find the blue color quite hard to read here, maybe leave it at white?
- Could we place the two SVGs in a way so that they are both visible at the same time, ideally without scrolling?
| @@ -146,45 +143,64 @@ bool ElementSVG::LoadSource() | |||
| { | |||
| source_dirty = false; | |||
| texture_dirty = true; | |||
| loading_inline_source = false; | |||
There was a problem hiding this comment.
Looks like this variable is only used in this function. Thus, we can move it from the class and make it local here. Generally, we should try to avoid introducing new state when possible.
| { | ||
| const String document_source_url = StringUtilities::Replace(document->GetSourceURL(), '|', ':'); | ||
| GetSystemInterface()->JoinPath(path, document_source_url, attribute_src); | ||
| GetSystemInterface()->JoinPath(directory, document_source_url, ""); |
There was a problem hiding this comment.
I just noticed that directory isn't really used for anything here. Looks like a previous copy/paste leftover from the Lottie plugin. Would be nice to clean that up while we're at it, thanks!
| ElementSVG::ElementSVG(const String& tag) : Element(tag) {} | ||
|
|
||
| ElementSVG::~ElementSVG() {} | ||
|
|
||
| bool ElementSVG::GetIntrinsicDimensions(Vector2f& dimensions, float& ratio) | ||
| { | ||
| if (source_dirty) | ||
| if (inline_resized || source_dirty) |
There was a problem hiding this comment.
Can you tell me more about this check? A resize shouldn't cause the intrinsic size to change, so it doesn't feel right to include it here.
| } | ||
|
|
||
| void ElementSVG::OnResize() | ||
| { | ||
| inline_resized = true; |
There was a problem hiding this comment.
I wonder if we can remove this variable too. Is the situation for inlines that different from src and how that dirties the geometry and texture already?
| // Get Inline SVG RML. | ||
| this->GetRML(svg_data); | ||
| // If the Inline SVG RML is empty, then report a error. | ||
| if (svg_data.empty() == true) |
There was a problem hiding this comment.
Minor style: Please avoid comparing to boolean literals. This also goes for the == false above.
| // Get Inline SVG RML. | ||
| this->GetRML(svg_data); | ||
| // If the Inline SVG RML is empty, then report a error. | ||
| if (svg_data.empty() == true) | ||
| { | ||
| Log::Message(Rml::Log::Type::LT_ERROR, "Could not load inline SVG data. Is the data invalid?"); | ||
| } | ||
| // reset the guard. | ||
| inline_resized = false; |
There was a problem hiding this comment.
The code here looks quite self-explanatory, which is great. But then the comments don't add much, so in my view they can be removed.
Seems a lot better 😂, to be honest, I was thinking of revamping the whole thing with this in mind because since we couldn't manipulate the properties as easily. If you also agree, I can close the PR, and keep on developing it with some guidance from you. |
|
Sounds good. I'll leave it to you whether you want to make a new PR, or keep updating this one. If so, you could mark this is a draft and then mark it as ready for review once you are happy with it. In any case, I'll of course be happy to help out, let me know if you have some questions. |
|
will close this one for now, and reopen a new pr once the new implementation is ready for feedback. |
This is a quick Pull Request to add "Inline" SVG support to RmlUI.
This is something that i was able to get up-and-running very quickly, so feel free to suggest any changes to the PR.
Below is a SVG Loaded entirely from rml:
