search

svg-net / SVG

Fork of the ms svg library
http://svg-net.github.io/SVG/
Microsoft Public License
1.16k stars 473 forks source link

Make IdManager property public for SvgDocument #527

Closed wieslawsoltes closed 5 years ago

wieslawsoltes commented 5 years ago

Description

Make IdManager property public for SvgDocument: https://github.com/vvvv/SVG/blob/816af8f7ab3b2ffd41913a2c464db8d9b69a5d38/Source/SvgDocument.cs#L90

Example data

You can not get ReferencedElement for SvgUse object. This is limiting usage and custom rendering. https://github.com/vvvv/SVG/blob/816af8f7ab3b2ffd41913a2c464db8d9b69a5d38/Source/Document%20Structure/SvgUse.cs#L28

Used Versions

master branch

mrbean-bremen commented 5 years ago

Can't you just use this.OwnerDocument.GetElementById(useElement.ReferencedElement) ?

wieslawsoltes commented 5 years ago

Can't you just use this.OwnerDocument.GetElementById(useElement.ReferencedElement) ? When I do that:

error CS1503: Argument 1: cannot convert from 'System.Uri' to 'string'
wieslawsoltes commented 5 years ago

This seems to work:

svgUse.OwnerDocument.GetElementById(svgUse.ReferencedElement.ToString());

Anyway to do some more testing.

wieslawsoltes commented 5 years ago

Other issue when trying to render use element: https://github.com/vvvv/SVG/blob/816af8f7ab3b2ffd41913a2c464db8d9b69a5d38/Source/Document%20Structure/SvgUse.cs#L170-L175 The Parent is readolny property.

mrbean-bremen commented 5 years ago

svgUse.OwnerDocument.GetElementById(svgUse.ReferencedElement.ToString());

Yes, correct - I got that wrong.

mrbean-bremen commented 5 years ago

Other issue when trying to render use element:

Sorry, didn't get that - you want to change the parent? Or use that code in your own?

wieslawsoltes commented 5 years ago

I want to render SvgUse on my own, but in the original SvgUse code before drawing reference element the parent is changed (after its rendered its restored) so the additional styles are applied. I can not do this is my code as I do not have access to internal _parent.

mrbean-bremen commented 5 years ago

Hm, I'm not sure it's the best idea to make Parent writable, as semantically it really is read-only. Maybe better add a public function that does that re-rendering?

wieslawsoltes commented 5 years ago

Hm, I'm not sure it's the best idea to make Parent writable, as semantically it really is read-only. Maybe better add a public function that does that re-rendering?

I am trying to solve this issue by my own right now, when I will find solution or hit another roadblock I will report here.

wieslawsoltes commented 5 years ago

So definitely need some method to set Parent to apply styles. Not sure how adding additional method for rendering would help, I am using different rendering back-end and I also want to use shape geometry created by my rendering engine.

mrbean-bremen commented 5 years ago

Ok, I see. Would like to have some third opinion here - maybe @H1Gdev or @gvheertum will have a look?

wieslawsoltes commented 5 years ago

@mrbean-bremen I have opened PR https://github.com/vvvv/SVG/pull/529 for further discussion. This issue can be closed as its solved.

gvheertum commented 5 years ago

Funny, I was browsing the issues today and also had a look at this one, but decided that I did not yet understand what was happening.

@wieslawsoltes If you alter the parent, this means that you are "tampering" with the tree of the document right? This feels a bit itchy if you ask me, since updating the parent means that you "move" a child between nodes. Won't the parent be updated if you add the element to the new parent as a child? In most tree-like solutions, the behavior is to define the children and not the parents (these are set automatically).

You can ofcourse use Reflection to bruteforce your way into the _parent property, but I wouldn't be surprised if that breaks rendering if not used correctly.

wieslawsoltes commented 5 years ago

Funny, I was browsing the issues today and also had a look at this one, but decided that I did not yet understand what was happening.

@wieslawsoltes If you alter the parent, this means that you are "tampering" with the tree of the document right? This feels a bit itchy if you ask me, since updating the parent means that you "move" a child between nodes. Won't the parent be updated if you add the element to the new parent as a child? In most tree-like solutions, the behavior is to define the children and not the parents (these are set automatically).

You can ofcourse use Reflection to bruteforce your way into the _parent property, but I wouldn't be surprised if that breaks rendering if not used correctly.

Well the issue is that its required to change parent to apply parent properties when rendering SvgUse element and its done that way internally, so in my opinion setting Parent should be allowed for user and its necessary to have valid rendering.

See the current code for SvgUse: https://github.com/vvvv/SVG/blob/850c9cbdb901ed9a0ee79617f6072fa33d094ca1/Source/Document%20Structure/SvgUse.cs#L169-L175

wieslawsoltes commented 5 years ago

So without totally abstracting the ISvgRenderer and by that I mean removing dependency from System.Drawing.* I don't see any other solution to implement different rendering sub-system and not completely rewriting current code. So my proposal's is based on my current experience with this library and my implementation. I am open to discussion and looking forward for the best solution possible.

For reference: https://github.com/vvvv/SVG/blob/850c9cbdb901ed9a0ee79617f6072fa33d094ca1/Source/Rendering/ISvgRenderer.cs#L7-L25

This is my current renderer (very early version and not complete): https://github.com/wieslawsoltes/Draw2D/blob/2107bf95c3439cbeb07b8648b96520ad39b76e21/src/SvgDemo/Renderer/SkiaSvgRenderer.cs#L13

gvheertum commented 5 years ago

Funny, I was browsing the issues today and also had a look at this one, but decided that I did not yet understand what was happening. @wieslawsoltes If you alter the parent, this means that you are "tampering" with the tree of the document right? This feels a bit itchy if you ask me, since updating the parent means that you "move" a child between nodes. Won't the parent be updated if you add the element to the new parent as a child? In most tree-like solutions, the behavior is to define the children and not the parents (these are set automatically). You can ofcourse use Reflection to bruteforce your way into the _parent property, but I wouldn't be surprised if that breaks rendering if not used correctly.

Well the issue is that its required to change parent to apply parent properties when rendering SvgUse element and its done that way internally, so in my opinion setting Parent should be allowed for user and its necessary to have valid rendering.

See the current code for SvgUse:

https://github.com/vvvv/SVG/blob/850c9cbdb901ed9a0ee79617f6072fa33d094ca1/Source/Document%20Structure/SvgUse.cs#L169-L175

Ah, now I understand what you mean. I thought the code you placed was your own, but I now realized it's something in the SVG library. So basically, you want to build extended behavior and want to have the same access as the library to do so (the element is internal, so the svg lib can access the data itself asif it's a public)? I was a bit puzzled by the code example, but if I understand correctly you want to be able to set the parent to make sure that calls like the RenderElement use the correct information.

Still not fully sure if having a settable parent is the best way to go, but I get where you are going. There might be some desire for some "parent/child" alteration be available for users of the library (I often tamper with the XML before parsing it to the SVG lib). The SvgCollection already has some logic for this (and calls event handlers and such), so perhaps we should extend the SvgElement with some tree-alteration logic. This allows us to also validate if the tree does not get corrupted, because if you for example link items to each other as parents this might result in endless looping when iterating through the tree.

wieslawsoltes commented 5 years ago

Yes I want same access to internals (where its necessary) as the library.

The issue comes from the fact that SgvUse element is referencing other SvgElements elements. So this is by design (Svg standard dictates that). The referenced element by itself can by drawn many times (like SvgSymbol) and can have many parents and each time can inherit different attributes depending on currently rendered element. So currently Parent property needs to be changed each time so we conform to the standard.

wieslawsoltes commented 5 years ago

Actually there is different solution to this problem, excerpt from Svg standard.

Basically as I understand the spec, the SvgUse should be converted to SvgGroup in all cases, this solves the problem with Parent.

If the ‘use’ element references a ‘symbol’ element:

In the generated content, the ‘use’ will be replaced by ‘g’, where all attributes from the ‘use’ element except for ‘x’, ‘y’, ‘width’, ‘height’ and ‘xlink:href’ are transferred to the generated ‘g’ element. An additional transformation translate(x,y) is appended to the end (i.e., right-side) of the ‘transform’ attribute on the generated ‘g’, where x and y represent the values of the ‘x’ and ‘y’ attributes on the ‘use’ element. The referenced ‘symbol’ and its contents are deep-cloned into the generated tree, with the exception that the ‘symbol’ is replaced by an ‘svg’. This generated ‘svg’ will always have explicit values for attributes ‘width’ and ‘height’. If attributes ‘width’ and/or ‘height’ are provided on the ‘use’ element, then these attributes will be transferred to the generated ‘svg’. If attributes ‘width’ and/or ‘height’ are not specified, the generated ‘svg’ element will use values of '100%' for these attributes.

If the ‘use’ element references an ‘svg’ element:

In the generated content, the ‘use’ will be replaced by ‘g’, where all attributes from the ‘use’ element except for ‘x’, ‘y’, ‘width’, ‘height’ and ‘xlink:href’ are transferred to the generated ‘g’ element. An additional transformation translate(x,y) is appended to the end (i.e., right-side) of the ‘transform’ attribute on the generated ‘g’, where x and y represent the values of the ‘x’ and ‘y’ attributes on the ‘use’ element. The referenced ‘svg’ and its contents are deep-cloned into the generated tree. If attributes ‘width’ and/or ‘height’ are provided on the ‘use’ element, then these values will override the corresponding attributes on the ‘svg’ in the generated tree.

Otherwise:

In the generated content, the ‘use’ will be replaced by ‘g’, where all attributes from the ‘use’ element except for ‘x’, ‘y’, ‘width’, ‘height’ and ‘xlink:href’ are transferred to the generated ‘g’ element. An additional transformation translate(x,y) is appended to the end (i.e., right-side) of the ‘transform’ attribute on the generated ‘g’, where x and y represent the values of the ‘x’ and ‘y’ attributes on the ‘use’ element. The referenced object and its contents are deep-cloned into the generated tree.

https://www.w3.org/TR/SVG11/struct.html#UseElement

H1Gdev commented 5 years ago

It is different from the first issue...

Looking at #529, it seems that only public setter are provided, but is there no fundamental solution ? At first glance it seems that nothing has been solved.

Basically as I understand the spec, the SvgUse should be converted to SvgGroup in all cases, this solves the problem with Parent.

How is this solution ?

wieslawsoltes commented 5 years ago

It is different from the first issue...

Looking at #529, it seems that only public setter are provided, but is there no fundamental solution ? At first glance it seems that nothing has been solved.

Its solves the styling issue when rendering SvgUse. The Parent is different for each referenced element from SvgUse. Currently this is only accessible internally.

Basically as I understand the spec, the SvgUse should be converted to SvgGroup in all cases, this solves the problem with Parent.

How is this solution ?

This is how svg spces defines processing of SvgUse elements. Basically they are converted to SvgGroup (usually deep copy) and special handling of attributes. This is best solution and according to spec, but its probably huge changes and breaking one. So I prefer to enable access to Parent property setter and solve without braking too much or nothing. Anyway, currently I have solved this by using reflection and setting internal _parent field.