svgdotjs / svg.draw.js

An extension of svg.js which allows to draw elements with mouse
MIT License
239 stars 57 forks source link

Removing points in a Polyline is rewriten by svg.draw.js #17

Open dotnetCarpenter opened 7 years ago

dotnetCarpenter commented 7 years ago

Take the following code snippet, where peek(lines) returns the current Polyline and info() just prints to console.log:

function cancel() {
    const l = peek(lines)
    const points = l.attr('points').replace(/(\s\d+,\d+){2}$/, '')

    info(l.attr('points'))
    info(l.array().value.reduce( (a1,a2) => a1.concat(a2) ).join(' '))

    info(points)
    l.attr('points', points)
    l.array(new SVG.PointArray(
        l.array().value.slice(0, l.array().value.length - 3)
    ))

    info(l.attr('points'))
    info(l.array().value.reduce( (a1,a2) => a1.concat(a2) ).join(' '))

    /*peek(lines)
        .draw('cancel')*/
}

Live example: https://github.com/dotnetCarpenter/mua

Double click on the SVG element and add some points (single click) and then press ctrl + c.

If you look in the devtool console, you can see the points, both via the points attribute and via the .array() method. If I remove the last two points (the last is for animation and the second last is the last clicked point), and update both the polyline's points attribute and via the polyline's .array() method, svg.draw.js will still remember the point array and recreate the removed points.

Fuzzyma commented 7 years ago

The corresponding code to this behavior should be in the calc function. However as you can see:

        calc:function (e) {
            var arr = this.el.array().valueOf();
            arr.pop();

            if (e) {
                var p = this.transformPoint(e.clientX, e.clientY);
                arr.push(this.snapToGrid([p.x, p.y]));
            }

            this.el.plot(arr);

        },

The array is pulled from the element every time. Nothing is cached. Iam not entirely sure what you are doing there anyway:

        // why dont you use `plot` which updates the internal array
    l.attr('points', points)

       // `array()` is no setter. When you want to set the array use plot again
    l.array(new SVG.PointArray(
        l.array().value.slice(0, l.array().value.length - 3)
    ))

So I guess the fix to your problem is: Use plot! (or delete the array with clear() after changing points directly on the element - but no - just use plot)

Code that should work:

        // no need to use PointArray - the parse method also accepts arrays
    l.plot(l.array().valueOf().slice(0, -2)) // slice accepts negative indices as well
dotnetCarpenter commented 7 years ago

You're right!

Got a version up now where ctrl + c removes the last point and last circle. The only syntax I could get working for removing the circles (calling .drawCircles() with less points), is l.draw('drawCircles', points.slice(0, -1)).

The entire cancel function then becomes:

function cancel() {
    const l = peek(lines)

    if(!l) return

    // remove last point
    const points = l.array().valueOf().slice(0, -1)

    // update polyline with new points
    l.plot(points)

    // redraw circles with new points
    //l.draw.plugins.polyline.drawCircles.call(l.draw.plugins.polyline, points)
    l.draw('drawCircles', points.slice(0, -1))

    if( l.array().valueOf().length === 1 ) {
        l.draw('cancel')
        lines.pop()
    }
}

Should .draw('drawCircles') also be documented? Could be in the same section as we're talking about in #16.

Anyway, I think that using .plot() to update the path/points should be clear. If you want, I'll think of a way to describe it and create a PR?

Fuzzyma commented 7 years ago

Mh did you try calling the update function instead of the drawCircle function? I feel like this circle thingy should not be public api.

dotnetCarpenter commented 7 years ago

I just did a really quick test with l.draw('update', points.slice(0, -1)) and Chrome gives me Error: attribute points: Expected number, "177,110 291,50 NaN,NaN". and Firefox give me TypeError: Value being assigned to SVGPoint.x is not a finite floating-point value.

I'll be back in an hour to investigate further...

Fuzzyma commented 7 years ago

Update does not expect an argument. Don't pass the points!

dotnetCarpenter commented 7 years ago

I tried that and it does nothing. Basically I need to remove the last point from .array().valueOf() and the last 2 points when redrawing the circles.

// remove last point
const points = l.array().valueOf().slice(0, -1)

// update polyline with new points
l.plot(points)

// redraw circles with new points
l.draw('drawCircles', points.slice(0, -1))

// update the polyline to end at the mouse position
l.draw('update')

I'm pretty sure that it works by .plot() removing the last point in _memory, .draw('drawCircles', points.slice(0, -1)) removes all circles and draw only N-2 points (the animation point and the last clicked point) and lastly .draw('update') in turn calls calc() which updates the polyline to the mouse pointer.

The only other way I can think of, that let me do this, is by creating a plug-in for svg.draw.js.

Fuzzyma commented 7 years ago

I guess I will remove the need to pass any argument to drawCircles. It will just pull the needed points from the element instead. That would make drawCircles less confusing and could be part of the public api

dotnetCarpenter commented 7 years ago

Unfortunately that will not work. I have to (as in it is my use-case) remove 2 points when drawing the circles or it looks weird. The circle for the last animation point will be drawn, if it doesn't have one point less. So I need an API where I can change the circles. To that end, .draw('drawCircles', points) worked well. So please don't remove it.

I can upload a demo, if you need to see it.

dotnetCarpenter commented 7 years ago

If you clone https://github.com/dotnetcarpenter/mua/ you can open index.html and see the issue. In the master branch it's working (with current svg.draw.js release) and in the newDraw branch (with the version in your master branch and without sending points to drawCircles) it looks weird when you use ctrl +z to undo the last point.

Fuzzyma commented 7 years ago

Well the parameter free version would just draw all points except the last. This would work, wouldn't it?