Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add a way to delete vertices with the Modify interaction #1283

Merged
merged 3 commits into from Mar 28, 2014

Conversation

Projects
None yet
4 participants
Owner

ahocevar commented Nov 18, 2013

After this change, vertices can be deleted by simply clicking on
them. This is the same behaviour as e.g. in geojson.io.

Owner

ahocevar commented Dec 2, 2013

cc @oterral - I heard you'll be porting the editing tools to the vector-api branch.

Contributor

oterral commented Dec 4, 2013

Thx @ahocevar I'll try to integrate that in the vector-api

Owner

tschaub commented Dec 10, 2013

I think this is nice behavior, but I'm seeing some assertion errors when trying to delete shared vertices. I haven't narrowed it down to an easily repeatable sequence, but I periodically see assertion errors with this stack:

Uncaught AssertionError: Assertion failed asserts.js:103
goog.asserts.doAssertFailure_ asserts.js:103
goog.asserts.assert asserts.js:118
ol.structs.RBush.remove rbush.js:593
ol.interaction.Modify.removeVertex_ modifyinteraction.js:504
ol.interaction.Modify.handleClick_ modifyinteraction.js:459
ol.interaction.Modify.handleMapBrowserEvent modifyinteraction.js:441
ol.Map.handleMapBrowserEvent map.js:765
goog.events.EventTarget.fireListeners eventtarget.js:284
goog.events.EventTarget.dispatchEventInternal_ eventtarget.js:380
goog.events.EventTarget.dispatchEvent eventtarget.js:198
ol.MapBrowserEventHandler.relayEvent_ mapbrowserevent.js:474
goog.events.fireListener events.js:730
goog.events.handleBrowserEvent_ events.js:851

And here is a picture:

delete

Owner

ahocevar commented Dec 10, 2013

Thanks for spotting this @tschaub. I'll investigate this further and try to come up with a fix.

Owner

ahocevar commented Dec 11, 2013

Note to /self: double-check iterator variable name in nested loop.

@tschaub: The issue should be fixed now.

Owner

ahocevar commented Dec 11, 2013

@tschaub, the above was only part of the fix. Looks like there is a 2nd issue. Working on that one now.

Owner

ahocevar commented Dec 11, 2013

@tschaub The "issue" shown in the picture is unrelated to the failing assertion. The picture shows what happens when a vertex is deleted from multiple geometries where neighboring segments have different vertices on each geometry. Regarding the assertion, it seems like an issue with RBush's extent lookup. But so far I have been unable to nail it down. Doing more research now.

Owner

tschaub commented Dec 11, 2013

The "issue" shown in the picture is unrelated to the failing assertion

Right, I just included the picture to show you where I was editing. I didn't mean to imply that the picture itself demonstrated the issue. Apologies for the confusion.

Owner

ahocevar commented Dec 11, 2013

No worries @tschaub - the picture helped indeed to find a way to consistently reproduce the RBush problem.

Owner

ahocevar commented Dec 19, 2013

For what it's worth, now that #1373 is fixed, vertex deletion works as expected again.

Owner

ahocevar commented Mar 14, 2014

This needs to be ported to the new Modify interaction. I'll be doing this shortly.

Owner

ahocevar commented Mar 20, 2014

Updated. Thanks for any review.

Contributor

oterral commented Mar 27, 2014

Tested on safari/chrome mac. Works great!!!

Owner

tschaub commented Mar 27, 2014

I'm wondering if it makes sense to require a more explicit action for vertex deletion. No ideas at the moment, but I'm concerned that people might inadvertently delete points when they are trying to do something else. For example, if you are working with small features and are trying to select a feature adjacent to an already selected feature, you can end up deleting points instead of selecting the new feature (this is related to the comments in #1914).

Part of this could be ameliorated by a smaller pixelTolerance in the modify interaction (10 pixels makes accidental deletes on selection less frequent for me).

Any thoughts @ahocevar?

Owner

ahocevar commented Mar 27, 2014

I think reducing the pixelTolerance to 10 pixels makes sense, but I want to test on mobile hidpi devices first to see how this affects usability.

As an alternative to this, we could introduce a delete modifier (Meta/Alt key?). But that might raise usability issues too.

If we add an undo functionality, accidental vertex deletion should not be a problem any more.

Owner

tschaub commented Mar 27, 2014

A lower tolerance plus a promise of undo in the future is probably enough. We could also add support for a delete condition in the future, but leave the default always.

ahocevar added some commits Mar 20, 2014

@ahocevar ahocevar Add a way to delete vertices with the Modify interaction
After this change, vertices can be deleted by simply clicking on
them. This is the same behaviour as e.g. in geojson.io.
e612330
@ahocevar ahocevar Lower pixel tolerance to avoid accidental vertex deltion b9cd512
Owner

ahocevar commented Mar 28, 2014

Ticket for undo created: #1919.

@ahocevar ahocevar added a commit that referenced this pull request Mar 28, 2014

@ahocevar ahocevar Merge pull request #1283 from ahocevar/delete-vertex
Add a way to delete vertices with the Modify interaction
37eea93

@ahocevar ahocevar merged commit 37eea93 into openlayers:master Mar 28, 2014

1 check passed

default The Travis CI build passed
Details

@ahocevar ahocevar deleted the ahocevar:delete-vertex branch Mar 28, 2014

@elemoine elemoine commented on the diff Mar 28, 2014

src/ol/interaction/modifyinteraction.js
@@ -64,6 +64,12 @@ ol.interaction.Modify = function(options) {
this.modifiable_ = false;
/**
+ * @type {ol.Coordinate}
+ * @private
+ */
+ this.lastVertexCoordinate_;
@elemoine

elemoine Mar 28, 2014

Owner

The property should be initialized (to null).

Owner

elemoine commented Mar 28, 2014

Sorry for the post-merge review. I just added a comment regarding the initialization of the lastVertexCoordinate_ property. Thanks.

Owner

ahocevar commented Apr 1, 2014

@elemoine lastVertexCoordinate_ is removed with 66fecc8e6a14b37661d76dae26e310a56a0bbfae, which is part of #1914.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment