trafficonese / leaflet.extras

Extra functionality for leaflet R package.
https://trafficonese.github.io/leaflet.extras/
GNU General Public License v3.0
216 stars 74 forks source link

Duplicated points to draw a polygon #37

Closed byzheng closed 7 years ago

byzheng commented 7 years ago

There are duplicated points (9) when I draw a polygon with four points.

This is the output of new_ply$geometry$coordinates after convert to a matrix

     [,1]      [,2]
 [1,] 152.3303 -27.56613
 [2,] 152.3303 -27.56613
 [3,] 152.3311 -27.56532
 [4,] 152.3311 -27.56532
 [5,] 152.3321 -27.56587
 [6,] 152.3321 -27.56587
 [7,] 152.3316 -27.56674
 [8,] 152.3316 -27.56674
 [9,] 152.3303 -27.56613

I expect 5 rows in the coordinates but 9 rows. The first 4 rows are duplicated.

bhaskarvk commented 7 years ago

Interesting, and thanks for reporting. I'll investigate

bhaskarvk commented 7 years ago

I'm not able to duplicate this bug. Here's a screenshot of what I see when the CREATED event is fired, and there are only 5 points for a 4 point polygon as expected.

Imgur

byzheng commented 7 years ago

I can confirm this problem is related with chrome (54.0.2840.99 m), not firefox. In my memory, I occasionally have the same problem before with chrome.

PS: How do you debug a shiny app as I cannot find draw-bindling.js in the console when I debug a shiny app?

bhaskarvk commented 7 years ago

Strange, I am using chrome and I don't see this. If this is indeed a sporadic issue, then it must be in leaflet + chrome combo, coz all I really do is layer.toGeoJSON() in the event captured when draw actions happen. I don't manually construct the object.

I didn't use Shiny above, I just opened my widget html in chrome and debugged it. I think @jcheng5 might be able to help debug Shiny apps.

byzheng commented 7 years ago

I figure out the same method (widget html) to debug leaflet.extras packages. It seems this problem appears in a few days, then disappear, and come back again. Could be related with chrome version.

bhaskarvk commented 7 years ago

I'm closing this then. Will reopen if more people face the same issue, but even in that case I think this needs to be a bug on leaflet's layer.toGeoJSON() call.

byzheng commented 7 years ago

After debugging, this problem is related with leaflet 0.7.*. Since 0.7.3, leaflet fires 2 mouseup event when a single mouse clicking in my chrome browser. 0.7.2 is working fine to me.

bhaskarvk commented 7 years ago

Interesting, only in Chrome ? I'm on Chrome 53 and I don't see this. Also do two mouseup events result in two 'draw:created' events ?

byzheng commented 7 years ago

Finally I figure out the problem. It is related with my laptop Dell E7250 which have a keyboard and touch screen. I believe Leaflet.draw (0.1.0 in the folder name or 0.4.3 in the leaflet.draw-src.js) has some bugs to handle touch screen. See the codes below from leaflet.draw-src.js

_onTouch: function (e) {
    // #TODO: use touchstart and touchend vs using click(touch start & end).
    if (L.Browser.touch) { // #TODO: get rid of this once leaflet fixes their click/touch.
        this._onMouseDown(e);
        this._onMouseUp(e);
    }
},

In each time, a mouse event is fired from the mouse and touch screen, even there is only one mouse down and up. This behaviour causes the duplicated points in some cases.

I have tried the latest version of leaflet.draw with this example including all js files (https://leaflet.github.io/Leaflet.draw/docs/examples-0.7.x/full.html). I don't find any duplicated points when I draw polygons until now.

Do you have a plan to upgrade to the latest version of leaflet.draw?

bhaskarvk commented 7 years ago

Hmm, I am using draw 0.4. I even modified leaflet.path.drag and leaflet.styleeditor to work correctly with 0.4.

bhaskarvk commented 7 years ago

That example you listed, is using Leaflet JS 1.x, and we're still at 0.7.x. That could be the problem, not the version of Leaflet.draw

bhaskarvk commented 7 years ago

Ok I just checked and found this https://github.com/Leaflet/Leaflet.draw/commit/77a9fa25919c44488efd2392a61f51718136027c.

So there has been a new version pushed since I last integrated this plugin, I'll update the package soon.

bhaskarvk commented 7 years ago

I just pushed a commit with latest Laflet.draw, please rebuild and retest

byzheng commented 7 years ago

Sorry for the confusing. I actually use leaflet 0.7.7 from leaflet package, the only replaced the leaflet.draw from github master branch. And work for me.

bhaskarvk commented 7 years ago

Did you try with my latest push, does it work correctly now ? I included the latest leaflet.draw in it, but I haven't incremented the version number of the leaflet.extras package.

byzheng commented 7 years ago

Thanks for your help. It is working now.