SVGLoader: Added fill-opacity style support. #15922

flo-Ty · 2019-03-07T08:21:21Z

Working example here: https://btevc.cf/repos/three.js/examples/webgl_loader_svg_update_1.html

examples/js/loaders/SVGLoader.js

WestLangley · 2019-03-07T17:03:52Z

yomboprime · 2019-03-08T00:11:22Z

Seems all right to me 👍

yomboprime · 2019-03-08T02:53:55Z

Now that I think on it, the SVGLoader code is adding the fillOpacity member to the ShapePath instances.
Is that okay?
Previously it was only used the color member, which is defined in the ShapePath class.

WestLangley · 2019-03-08T03:12:38Z

I am not sure what is best... Someone who is more-familiar with this will have to speak up.

yomboprime · 2019-03-09T20:34:07Z

(I think @Mugen87 can throw light on this.)

According to mrdoob: #15170 (comment)

It seems the SVGLoader should not export the SVG tags, but better the SVG node itself, in the object ShapePath.userData.

So the SVGLoader should put path.userData = { node: node }. And fillOpacity should be obtained by the user with path.userData.node.getAttribute( 'fill-opacity' ) instead of path.fillOpacity.

Additionally I think new functions svgLoader.getRootNode() and svgLoader.getNodePathMap() (map from nodes to paths) could be called (after the SVG is loaded) if the SVG node tree is needed (see linked PR above)

Exporting the SVG tags allows the user to render things like strokes, which are a nice feature to have. For example, the tiger.svg has some shape outlines and not-closed paths that have got tags like stroke-color and stroke-width. They are not rendered currently.

To not break old user code, a new flag svgLoader.exportOnlyVisible or exportOnlyFillShapes, which defaults to true, could be added to let export (when false) the stroke paths which have the fill-color set to none and thus are currently non-visible and are not exported.

If this is done I can also implement a function that generates, given the path points and the stroke SVG properties, the stroke shape (with miter line joining and no linecaps). I can implement later the tags strokeLinejoin, strokeLinecap and strokeMiterLimit among others.

The stroke creation could also be useful for shapes generated from Font text.

flo-Ty · 2019-03-09T21:11:39Z

Okay, this is a little above my experience level, but I would be willing to put in the work to figure out how to do it if it would be useful.

yomboprime · 2019-03-09T21:52:57Z

Please, let's wait for some feedback. I am no authority here 😊

If my idea is approved, these are the changes:

Keep your changes to the parseStyle() function, they are all right.
Put path.userData = { node: node }; just after the line paths.push( path );.
Revert your other changes to the SVGLoader.

With only this, you can access in your application any SVG tag as I said: path.userData.node.getAttribute( 'fill-opacity' )

Later other tags can be added to the style and I can do a PR for the stroke creation.

About the node map, you can also add it if you want:

Add a new this.nodeMap = new Map(); instance variable to the constructor.
Add a new this.rootNode = null; instance variable to the constructor.
Put nodeMap.set( node, path ); just after the line paths.push( path );.
Put this.rootNode = xml.documentElement; just after the line parseNode( xml.documentElement, { fill: '#000' } );

That's all. A getter function for the map and another for the rootNode are optional.

WestLangley · 2019-03-10T01:24:53Z

To not break old user code

I wouldn't worry about that in this case. You can always add a migration note.

Please, let's wait for some feedback. I am no authority here

...and I'll defer to @mrdoob and @Mugen87. :-)

Mugen87 · 2019-03-10T10:48:18Z

The suggestion of @mrdoob was to introduce userData to ShapePath which is also done in other classes of three.js. Exposing the entire node is probably the most flexible approach rather than exposing individual node properties over ShapePath.

The problem was that the OP of #15170 never implemented his feedback. This PR could be a good opportunity to finally do so 😉

flo-Ty · 2019-03-11T00:11:33Z

Okay great, I'll get started on implementing the changes @yomboprime suggested in the next couple of days then.

yomboprime · 2019-03-11T00:40:34Z

Another thought: As the node properties are not inherited between the nodes, I would also add the style to userData, i.e.:
path.userData = { node: node, style: style };

(The styles do inherit properties from upper nodes)

yomboprime · 2019-03-13T00:29:51Z

Looks good. I got already working the stroke creation. In some days I can make a PR.

I need to implement stroke-miterlimit to limit the miter that goes away in some places as you can see. Will implement also round and bevel joins and also linecaps.

flo-Ty · 2019-03-13T00:36:34Z

Awesome. 👍

flo-Ty · 2019-03-13T00:54:07Z

Woops, had left in the line this.rootNode = xml.documentElement; from the nodeMap suggestion. The final decision was to exclude the nodeMap correct?

yomboprime · 2019-03-13T00:55:11Z

Even if the map is out, the root node can be useful. I would leave it. (And declare it in the constructor also, please)

flo-Ty · 2019-03-13T01:06:49Z

K, how does that look?

yomboprime · 2019-03-13T01:20:51Z

All right. You keep toggling code 😊

flo-Ty · 2019-03-13T01:22:36Z

Haha, yeah, sorry, had a long day today. :)

examples/js/loaders/SVGLoader.js

mrdoob · 2019-03-15T00:44:07Z

@WestLangley @Mugen87 @yomboprime thanks for the help here guys!

mrdoob · 2019-03-15T00:44:30Z

@yomboprime these strokes are looking great!

yomboprime · 2019-03-15T01:26:05Z

@yomboprime these strokes are looking great!

I'm making progress... 😃

yomboprime · 2019-03-15T12:42:31Z

I've got the strokes working right.

It is a long function (720 js lines) in THREE.SVGLoader.pointsToStroke( points, style, arcDivisions ) which returns a BufferGeometry.

I could move it to another place like its own class StrokeGeometry in examples/js/geometries/. What do you think?

mrdoob · 2019-03-15T13:22:35Z

@yomboprime I was expecting that 😁 I would add it to SVGLoader so it's self contained

yomboprime · 2019-03-15T23:40:24Z

@yomboprime I was expecting that grin I would add it to SVGLoader so it's self contained

Do you mean that I have a verbose coding habit? 😃

yomboprime · 2019-03-15T23:46:49Z

@btevc You should also modify the svgloader example for the new change in returned data.

flo-Ty · 2019-03-15T23:48:19Z

@yomboprime Oh right, so far only updated my own example.

mrdoob · 2019-03-15T23:48:22Z

Do you mean that I have a verbose coding habit? 😃

Not at all! I just suspected converting strokes to geometry was going to be arduous 🙈

yomboprime · 2019-03-15T23:59:21Z

@btevc You should do:

loader.load( url, function ( data ) {
    var paths = data.paths;

And leave the for loop as it was before

flo-Ty · 2019-03-16T00:05:24Z

@yomboprime Ah, mkay. Will fix this shortly.

mrdoob · 2019-03-16T00:55:30Z

examples/webgl_loader_svg.html

@@ -161,9 +161,9 @@
 					group.position.y = 70;
 					group.scale.y *= - 1;

-					for ( var i = 0; i < paths.length; i ++ ) {
+					for ( key in paths['paths'] ) {


This seems over complicated...
You should be able to keep the previous code and just add these lines:

loader.load( url, function ( data ) {

var paths = data.paths; for ( var i = 0; i < paths.length; i ++ ) {

flo-Ty · 2019-03-16T01:52:01Z

So checks still not passing, anyone know what might be going on here?

yomboprime · 2019-03-16T01:59:41Z

So checks still not passing, anyone know what might be going on here?

It seems something related to webvr renderer files. I have no Idea, could be a Github bug.

Other PRs seem to be affected by the same webvr files. But the files in the repo do not contain the error.

yomboprime · 2019-03-16T05:40:12Z

Bonus: Stroke Font text. I copied the webgl_geometry_text_shapes example and converted the line text to stroke text with minor changes.

Mugen87 · 2019-03-16T11:59:35Z

So checks still not passing, anyone know what might be going on here?

Don't worry about that. The travis build was temporarily broken. Should be fixed now.

yomboprime · 2019-03-20T16:12:38Z

@Mugen87 Can I make my PR if it depends on this one? I'm not a git expert.

Mugen87 · 2019-03-20T17:15:29Z

I guess it's better to wait until @mrdoob approves this PR. It has to merged before yours in any event.

BTW: This PR here needs to be mentioned in the migration guide since the parameter of the onLoad() function has changed. User will have to adjust their code similar to webgl_loader_svg.

mrdoob · 2019-03-26T23:20:56Z

Thanks!

flo-Ty · 2019-03-28T05:26:55Z

@yomboprime @WestLangley @Mugen87 @mrdoob Thank you all for helping me through this PR, exciting to participate in this library, even if in a very small way.

Add fill-opacity style support to SVGLoader.js

c77a03d

WestLangley reviewed Mar 7, 2019

View reviewed changes

examples/js/loaders/SVGLoader.js Outdated Show resolved Hide resolved

Update SVGLoader.js

dfdf924

Update SVGLoader.js

793342e

Update SVGLoader.js

054340f

flo-Ty added 2 commits March 12, 2019 21:00

Update SVGLoader.js

65d1696

Update SVGLoader.js

7ccdbcf

Mugen87 approved these changes Mar 13, 2019

View reviewed changes

mrdoob reviewed Mar 15, 2019

View reviewed changes

examples/js/loaders/SVGLoader.js Outdated Show resolved Hide resolved

mrdoob added this to the r103 milestone Mar 15, 2019

mrdoob changed the title ~~Add fill-opacity style support to SVGloader.js~~ SVGLoader: Added fill-opacity style support. Mar 15, 2019

Updated parse() to return data object

bf9213c

Updated webgl_loader_svg.html to use data object

e7fbd20

mrdoob reviewed Mar 16, 2019

View reviewed changes

Simplified paths loop in webgl_loader_svg.html

5477340

mrdoob merged commit 00337d9 into mrdoob:dev Mar 26, 2019

yomboprime mentioned this pull request Mar 27, 2019

SVGLoader: Add stroke support #16077

Merged

SVGLoader: Added fill-opacity style support. #15922

SVGLoader: Added fill-opacity style support. #15922

Conversation

flo-Ty commented Mar 7, 2019 • edited Loading

WestLangley commented Mar 7, 2019

yomboprime commented Mar 8, 2019

yomboprime commented Mar 8, 2019 • edited Loading

WestLangley commented Mar 8, 2019

yomboprime commented Mar 9, 2019

flo-Ty commented Mar 9, 2019

yomboprime commented Mar 9, 2019 • edited Loading

WestLangley commented Mar 10, 2019

Mugen87 commented Mar 10, 2019 • edited Loading

flo-Ty commented Mar 11, 2019

yomboprime commented Mar 11, 2019

yomboprime commented Mar 13, 2019

flo-Ty commented Mar 13, 2019

flo-Ty commented Mar 13, 2019

yomboprime commented Mar 13, 2019

flo-Ty commented Mar 13, 2019

yomboprime commented Mar 13, 2019

flo-Ty commented Mar 13, 2019

mrdoob commented Mar 15, 2019

mrdoob commented Mar 15, 2019

yomboprime commented Mar 15, 2019 • edited Loading

yomboprime commented Mar 15, 2019

mrdoob commented Mar 15, 2019

yomboprime commented Mar 15, 2019

yomboprime commented Mar 15, 2019

flo-Ty commented Mar 15, 2019 • edited Loading

mrdoob commented Mar 15, 2019

yomboprime commented Mar 15, 2019 • edited Loading

flo-Ty commented Mar 16, 2019 • edited Loading

mrdoob Mar 16, 2019 • edited Loading

Choose a reason for hiding this comment

flo-Ty commented Mar 16, 2019

yomboprime commented Mar 16, 2019 • edited Loading

yomboprime commented Mar 16, 2019

Mugen87 commented Mar 16, 2019

yomboprime commented Mar 20, 2019

Mugen87 commented Mar 20, 2019 • edited Loading

mrdoob commented Mar 26, 2019

flo-Ty commented Mar 28, 2019

flo-Ty commented Mar 7, 2019 •

edited

Loading

yomboprime commented Mar 8, 2019 •

edited

Loading

yomboprime commented Mar 9, 2019 •

edited

Loading

Mugen87 commented Mar 10, 2019 •

edited

Loading

yomboprime commented Mar 15, 2019 •

edited

Loading

flo-Ty commented Mar 15, 2019 •

edited

Loading

yomboprime commented Mar 15, 2019 •

edited

Loading

flo-Ty commented Mar 16, 2019 •

edited

Loading

mrdoob Mar 16, 2019 •

edited

Loading

yomboprime commented Mar 16, 2019 •

edited

Loading

Mugen87 commented Mar 20, 2019 •

edited

Loading