Crashes when appending defs for a polygon or path about rough HOT 14 CLOSED

That's cool. I will check in a fix soon anyway because clearly my code was sloppy in this area. the whole
defs! thing was just wrong.

from rough.

Awesome, that fixed it. Thanks!

from rough.

So, I had some time to debug it a bit. On the RoughSVG class you have a getter defs that can return this._defs or null:

        get defs() {
            if (hasDocument$1) {
                if (!this._defs) {
                    const doc = this.svg.ownerDocument || document;
                    const dnode = doc.createElementNS('http://www.w3.org/2000/svg', 'defs');
                    if (this.svg.firstChild) {
                        this.svg.insertBefore(dnode, this.svg.firstChild);
                    }
                    else {
                        this.svg.appendChild(dnode);
                    }
                    this._defs = dnode;
                }
            }
            return this._defs || null;
        }

A bit further along, in the path2Dpattern case of the draw() method you're calling this.defs.appendChild(pattern);. Which will fail if defs has returned null. Which is what I'm encountering.

The reason that defs returns null is because hasDocument$1 is false, which it is because you're checking for a global (document) which isn't available in the environment I'm using rough in (node).

So a quick fix would be to omit the hasDocument$1 check. You're defaulting to this.svg.ownerDocument in the getter, which works fine in node with jsdom. In the browser document will still be available, so no changes there.

This is quite similar to the other bug I encountered earlier, which was also relying on certain globals (self). I don't know if it's your intention to decouple this library from any specific environment, but if it is I'd recommend allowing the user to pass in the variables you're expecting (like document for example). You could always fall back to the globals for backwards compatibility. Because in this case I'd have to pollute the global scope for rough to work as expected.

from rough.

I modified the filling of polygons to user pattern fills. This is a side effect of that. I did that because my normal fill algorithm doesn't work reliably for concave polygons. I'll fix this shortly, Contemplating possibilities. It will affect Path renderings as well.

from rough.

Just so I better understand, can you explain how you're using this in node?

from rough.

Btw, have you looked at toPath feature in generator. https://github.com/pshihn/rough/wiki/RoughGenerator#svg-path-info

It avoids actual drawing of the shape, so you get the path information and you can render it however you like.

from rough.

I can fix this, as you suggested for jsdom. However, there are cases where the 'fill' may not be properly rendered as it cannot compute the actual size of the path which is needed for path fills. I'll optimize it as much as I can. Just giving heads up.

On a different note, if you are filling with 'transparent', you might as well not have the 'fill' flag in the options. This will increase your performance. There's not point computing hachures if they are not visible.

from rough.

Btw, have you looked at toPath feature in generator. https://github.com/pshihn/rough/wiki/RoughGenerator#svg-path-info

Yeah I was thinking of that. I didn't know that it also dealt with patterns, seems like a good solution.

Just so I better understand, can you explain how you're using this in node?

I'm working on a tool to convert an entire svg with rough: https://github.com/ismay/coarse. Reason is I have some other command line tools for generating svgs, and I wanted to be able to apply rough to an entire svg from the command line. I have a PR with a failing test here: https://github.com/ismay/coarse/pull/11

from rough.

Maybe instead of modifying the code to work in all environments, you could solve this by only supporting rendering outside of the browser with RoughGenerator. I knew of the generator api, but I kind of forgot about it as the regular rough api was what I first encountered in the docs.

I wouldn't mind using the roughgenerator api server-side, and for you as a maintainer it might also be easier to only allow use outside of the browser with that api. In that case it would be nice to state clearly that only the roughgenerator api works outside of the browser, if that isn't already mentioned.

from rough.

Yes that was the purpose of RoughGenerator though I never spent time actually testing it under different scenarios. Another use was to use generator in background workers where DOM does not exist - something common with server side rendering.

from rough.

Yes that was the purpose of RoughGenerator though I never spent time actually testing it under different scenarios. Another use was to use generator in background workers where DOM does not exist - something common with server side rendering.

Ok cool, I'll use the RoughGenerator api to fix this PR: https://github.com/ismay/coarse/pull/11. If that fixes it you'll also have a bit of proof that most of that API works in node (as I'm testing all available svg shapes, but not all possible roughjs shapes). Thanks for the feedback!

from rough.

No problem, I can imagine that the latest changes were quite the refactor. I'll let you know if I encounter anything with the generator api.

from rough.

i know https://emeeks.github.io/semiotic/ is using generator.toPaths to create PathInfo objects and then using those to create its own SVG nodes.

from rough.

Fixed those changes: #76
It may solve your immediate problem.
Fixes are in v2.2.2

from rough.