Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Cannot handle post-defined elements & reused <use> elements #170

Closed
YishiMichael opened this issue Mar 14, 2022 · 8 comments
Closed

Comments

@YishiMichael
Copy link

YishiMichael commented Mar 14, 2022

Summary Description

There are some problems when parsing elements which refer to other elements defined later, as well as elements with a transform parameter which are reused for multiple times. The latter of them may have been mentioned in #169.

Additional Details

When trying to deal with 3b1b/manim#1760, I notice that svgelements fails to handle the following svg file:

<?xml version='1.0' encoding='UTF-8'?>
<!-- This file was generated by dvisvgm 2.11.1 -->
<svg version='1.1' xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink' width='30.033152pt' height='6.789661pt' viewBox='85.143591 -71.954169 30.033152 6.789661'>
<defs>
<path id='g4-65' d='M5.378384 0L5.427878-.239223L5.114414-.26397C4.858693-.288717 4.825697-.404204 4.784452-.742415L4.174022-5.708346H3.588339L2.202498-3.274875C1.781796-2.540709 1.097124-1.3116 .791909-.816656C.52794-.387706 .387706-.296966 .131985-.272219L-.140234-.239223L-.189728 0H1.666309L1.715803-.239223L1.262105-.280468C1.097124-.296966 1.080626-.412453 1.154868-.585683C1.427087-1.113622 1.699305-1.649811 2.00452-2.202498H3.852309L4.042037-.602181C4.066784-.362958 4.000792-.296966 3.835811-.280468L3.398611-.239223L3.349116 0H5.378384ZM3.811063-2.515962H2.169501C2.606701-3.332618 3.060399-4.141026 3.505848-4.941184H3.522346L3.811063-2.515962Z'/>
<use id='g6-65' xlink:href='#g3-65' transform='scale(1.166668)'/>
<path id='g3-65' d='M5.361886 0V-.239223C4.883441-.280468 4.792701-.305215 4.644218-.734166L2.895418-5.708346H2.284988L1.418837-3.266626C1.163117-2.548958 .816656-1.559071 .52794-.808407C.354709-.362958 .280468-.26397-.239223-.239223V0H1.57557V-.239223L1.146619-.280468C.899147-.305215 .8744-.387706 .940392-.61043C1.080626-1.105373 1.253856-1.616815 1.443585-2.202498H3.291373L3.84406-.626928C3.92655-.387706 3.885305-.296966 3.621335-.272219L3.250128-.239223V0H5.361886ZM3.192384-2.515962H1.550822C1.814792-3.340867 2.103509-4.149275 2.350981-4.875191H2.375728L3.192384-2.515962Z'/>
</defs>
<g id='page1'>
<use x='85.143591' y='-65.164508' xlink:href='#g4-65'/>
<use x='90.669586' y='-65.164508' xlink:href='#g6-65'/>
<use x='96.801578' y='-65.164508' xlink:href='#g6-65'/>
<use x='102.93357' y='-65.164508' xlink:href='#g6-65'/>
<use x='109.065562' y='-65.164508' xlink:href='#g6-65'/>
</g>
</svg>

There're two issues popping out. Firstly, the id g6-65 uses glyph g3-65, which is defined after it, making svgelements cannot recognize the element and returns SVGElement instances instead of Paths. Secondly, if I switch these two lines, from the result of parsing it can be noticed that the g6-65 elements are really far apart, and are even not aligned horizontally given the y attributes are equal. The latter of them may have been mentioned in #169.

@tatarize
Copy link
Member

This has been noticed before. If the use is defined in a defs at the time everything is good. But, the would-be look ahead parts where the use in defs uses the thing not yet defined is a problem. Specifically #160 is intended to address this based on #156. The only thing to do is pre-parse the tree and have access to the entire shadow-dom tree before attaching the use elements.

It's on the agenda but is a bit of a rarer use case. It does happen and use objects referencing future objects just like css sheets affecting objects in the past, isn't quite there because of the ordering of the parsing.

@tatarize
Copy link
Member

tatarize commented Sep 6, 2022

This was fixed in #160.

Specifically it should handle your case. Even when that ordering worked it would get the element with the get by id and have used the same use id. Now it should play back the relevant portions of the xml within a use object correctly.

@YishiMichael
Copy link
Author

Many thanks for your help! I tried with the svg above and found all use elements are mapped to their corresponding elements. However, I noticed that the transform attributes of these elements are wierdly identity matrices, making these shapes completely overlapping with each other, rather than arranged horizontally as the x and y attributes suggest.
Parsing result:
image

Expected result:
image

I wonder if I've missed any method to properly obtain transformed positions, or any bug occurs somewhere.

@tatarize
Copy link
Member

tatarize commented Sep 7, 2022

It appears that I missed a spot. The x, y values are supposed to override the other values. So if you set x, y values, it's supposed to compound them in a particular way that is currently wrong. I'll check into correcting that.

@YishiMichael
Copy link
Author

When briefly scanning the code, I came up with two little suggestions.

  1. There's a reference for variable pi in function RoundShape._ramanujan_length, but is not imported definitely. It should be changed to tau / 2.
  2. I guess maybe Use class can inherit Group directly, as Use is itself a container. This would also help when using SVG.elements method to expand elements contained inside Use instances.

@tatarize
Copy link
Member

tatarize commented Sep 7, 2022

  1. Good catch. I only provisionally imported pi to provide tau for versions py<=3.5
  2. I tend to mimic the DOM classes since it gives me fewer problems. https://www.w3.org/TR/SVG11/struct.html#UseElement https://www.w3.org/TR/SVG11/struct.html#Groups It looks like use is not a container element or subclass of group. Though both are structural elements.

I did expand the Use instances a bit to include the x, y, height, and width specifically because I needed those to not propagate per spec.

This should be fixed in
#193

image

tatarize added a commit that referenced this issue Sep 7, 2022
@tatarize
Copy link
Member

tatarize commented Sep 7, 2022

Okay, should be fixed in 1.8.1

@YishiMichael
Copy link
Author

Thanks a lot! Now the result is as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants