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

SVGLoader: Implement 'defs' and 'use' nodes #20209

Merged
merged 13 commits into from
Sep 1, 2020
Merged

SVGLoader: Implement 'defs' and 'use' nodes #20209

merged 13 commits into from
Sep 1, 2020

Conversation

yomboprime
Copy link
Collaborator

The defs node contains definitions to use later by the use node. It allows to use SVG files which contains svg clone objects.

Thanks to @TakumaKira for the expression to get the referenced node.

Fixes #14569

@yomboprime
Copy link
Collaborator Author

I've commited the jsm instead of the js, please wait while I fix this.

@yomboprime
Copy link
Collaborator Author

Done.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 31, 2020

It seems the test file Defs4 produce the following warning:

three.module.js:8077 THREE.Color: Unknown color url(#3-a)

Relevant for this PR?

@yomboprime
Copy link
Collaborator Author

It seems the test file Defs4 produce this warning:

three.module.js:8077 THREE.Color: Unknown color url(#3-a)

Is this relevant for this PR?

The fill=url(nodeId) seems another form of using other node. But as the used node is a gradient (not implemented), it doesn't affect here.

@yomboprime
Copy link
Collaborator Author

I will look into implementing the other form of referencing the node. It seems easy. The SVG spec says it is possible to reference external SVG files with url( svgFileUrl#nodeId ). I would implement access to local nodes, like in the example SVG (no url, just the node id).

@yomboprime
Copy link
Collaborator Author

After looking more at it I've seen the url() form is mainly used to apply gradients to shapes, so I think it wouldn't be of use to implement it.

@yomboprime
Copy link
Collaborator Author

I've added a console warning when url() is encountered. Should I just remove that token in the SVG sample file?

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 31, 2020

Should I just remove that token in the SVG sample file?

I think it's okay to keep it 👍.

@Mugen87 Mugen87 added this to the r121 milestone Aug 31, 2020
@@ -674,11 +698,11 @@ THREE.SVGLoader.prototype = Object.assign( Object.create( THREE.Loader.prototype
var cxp = q * rx * y1p / ry;
var cyp = - q * ry * x1p / rx;

// Step 3: Compute (cx, cy) from (cx′, cy′)
// Step 3: Compute (cx, cy) from (cx′, cy′)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

var cx = Math.cos( x_axis_rotation ) * cxp - Math.sin( x_axis_rotation ) * cyp + ( start.x + end.x ) / 2;
var cy = Math.sin( x_axis_rotation ) * cxp + Math.cos( x_axis_rotation ) * cyp + ( start.y + end.y ) / 2;

// Step 4: Compute θ1 and Δθ
// Step 4: Compute θ1 and Δθ
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What codification should have the source .js files? UTF-8 or other?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be UTF-8. Maybe modularize.js doesn't currently support that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe modularize.js doesn't currently support that?

But the problem is in the .js, not the jsm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested, and modularize seems to behave correctly.

@@ -1092,144 +1118,158 @@ THREE.SVGLoader.prototype = Object.assign( Object.create( THREE.Loader.prototype

function parseNodeTransform( node ) {

var transform = new THREE.Matrix3();
var transform = new Matrix3();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake. Fixed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, are you sure it's fixed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I must have failed to git add it.

@@ -684,11 +708,11 @@ SVGLoader.prototype = Object.assign( Object.create( Loader.prototype ), {
var cxp = q * rx * y1p / ry;
var cyp = - q * ry * x1p / rx;

// Step 3: Compute (cx, cy) from (cx′, cy′)
// Step 3: Compute (cx, cy) from (cx′, cy′)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still seeing encoding issues...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed also.

@mrdoob mrdoob merged commit 3f1cf4e into mrdoob:dev Sep 1, 2020
@mrdoob
Copy link
Owner

mrdoob commented Sep 1, 2020

Thanks!

@yomboprime yomboprime deleted the svgdef branch April 12, 2021 14:10
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

Successfully merging this pull request may close these issues.

SVGLoader: Missing SVG specs break correct shape rendering.
3 participants