svgdotjs / svg.draw.js

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

change property `p` for event from SVGPoint to SVG.Point - fixing #57 #58

Open dotnetCarpenter opened 4 years ago

dotnetCarpenter commented 4 years ago

Instead of doing:

const point = {x:0,y:0}
rect.on('drawstart', event => {
  const {x,y} = event.detail.p
  point.x = x
  point.y = y
})

You can now do:

let point
rect.on('drawstart', event => {
  point = event.detail.p.clone()
})

Please see #57 for more info.

dotnetCarpenter commented 4 years ago

The reason a native point was used here is that there simply wasnt no SVG.Point when I wrote this plugin :)

@Fuzzyma Should event.detail.m be an instance of SVG.Matrix then?

There is a small overhead of using the class instead of a native DOMMatrix (getScreenCTM()). But it makes sense that both properties are svg.js classes. I'm just not sure what improvement it will give. I don't have a use-case.

Fuzzyma commented 4 years ago

Alright, I will put this on the backlog ^^

dotnetCarpenter commented 4 years ago

I might have an hour now to fix this

dotnetCarpenter commented 4 years ago

Sorry, no I do not have time now. But I will try to fix this, this week.

Just to be clear. This can be merged when SVG.Matrix is being used instead of this.el.node.getScreenCTM().inverse() and use SVG.Point.transform(this.m) instead of this.p.native().matrixTransform(this.m).

And lastly, we can use this.m = this.el.getScreenCTM().inverse() instead of this.m = this.el.node.getScreenCTM().inverse().

I might throw in an example folder to manually test the new code is working...

Fuzzyma commented 4 years ago

One typo: in svg.js its screenCTM and not getScreenCTM. But no pressure. This plugin needs a rework at some point anyway. Github just launched its new notification dashboard and this PR came up. So I want this merged, release it and then make the whole plugin compatible to svg.js v3