Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Merged
merged 3 commits into from

4 participants

@ahocevar
Owner

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

@ahocevar
Owner

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

@oterral

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

@tschaub
Owner

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

@ahocevar
Owner

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

@ahocevar
Owner

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

@tschaub: The issue should be fixed now.

@ahocevar
Owner

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

@ahocevar
Owner

@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.

@tschaub
Owner

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.

@ahocevar
Owner

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

@ahocevar
Owner

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

@ahocevar
Owner

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

@ahocevar
Owner

Updated. Thanks for any review.

@oterral

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

@tschaub
Owner

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?

@ahocevar
Owner

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.

@tschaub
Owner

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
@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
@ahocevar
Owner

Ticket for undo created: #1919.

@ahocevar ahocevar merged commit 37eea93 into from
@ahocevar ahocevar deleted the branch
@elemoine elemoine commented on the diff
src/ol/interaction/modifyinteraction.js
@@ -64,6 +64,12 @@ ol.interaction.Modify = function(options) {
this.modifiable_ = false;
/**
+ * @type {ol.Coordinate}
+ * @private
+ */
+ this.lastVertexCoordinate_;
@elemoine Owner

The property should be initialized (to null).

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

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

@ahocevar
Owner

@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
Commits on Mar 28, 2014
  1. @ahocevar

    Add a way to delete vertices with the Modify interaction

    ahocevar authored
    After this change, vertices can be deleted by simply clicking on
    them. This is the same behaviour as e.g. in geojson.io.
  2. @ahocevar
  3. @ahocevar
This page is out of date. Refresh to see the latest.
Showing with 134 additions and 24 deletions.
  1. +1 −1  src/objectliterals.jsdoc
  2. +133 −23 src/ol/interaction/modifyinteraction.js
View
2  src/objectliterals.jsdoc
@@ -488,7 +488,7 @@
/**
* @typedef {Object} olx.interaction.ModifyOptions
* @property {number|undefined} pixelTolerance Pixel tolerance for considering
- * the pointer close enough to a vertex for editing. Default is 20 pixels.
+ * the pointer close enough to a vertex for editing. Default is 10 pixels.
* @property {ol.style.Style|Array.<ol.style.Style>|ol.feature.StyleFunction|undefined} style FeatureOverlay style.
* @property {ol.Collection} features The features the interaction works on.
*/
View
156 src/ol/interaction/modifyinteraction.js
@@ -64,6 +64,12 @@ ol.interaction.Modify = function(options) {
this.modifiable_ = false;
/**
+ * @type {ol.Coordinate}
+ * @private
+ */
+ this.lastVertexCoordinate_;
@elemoine Owner

The property should be initialized (to null).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ /**
* @type {ol.Pixel}
* @private
*/
@@ -81,7 +87,7 @@ ol.interaction.Modify = function(options) {
* @private
*/
this.pixelTolerance_ = goog.isDef(options.pixelTolerance) ?
- options.pixelTolerance : 20;
+ options.pixelTolerance : 10;
/**
* @type {Array}
@@ -401,6 +407,7 @@ ol.interaction.Modify.prototype.handlePointerDown = function(evt) {
for (i = insertVertices.length - 1; i >= 0; --i) {
this.insertVertex_.apply(this, insertVertices[i]);
}
+ this.lastVertexCoordinate_ = goog.array.clone(vertex);
}
return this.modifiable_;
};
@@ -457,11 +464,18 @@ ol.interaction.Modify.prototype.handlePointerDrag = function(evt) {
* @inheritDoc
*/
ol.interaction.Modify.prototype.handlePointerUp = function(evt) {
- var segmentData;
- for (var i = this.dragSegments_.length - 1; i >= 0; --i) {
- segmentData = this.dragSegments_[i][0];
- this.rBush_.update(ol.extent.boundingExtent(segmentData.segment),
- segmentData);
+ var geometry = this.vertexFeature_.getGeometry();
+ goog.asserts.assertInstanceof(geometry, ol.geom.Point);
+ if (goog.array.equals(this.lastVertexCoordinate_,
+ geometry.getCoordinates())) {
+ this.removeVertex_();
+ } else {
+ var segmentData;
+ for (var i = this.dragSegments_.length - 1; i >= 0; --i) {
+ segmentData = this.dragSegments_[i][0];
+ this.rBush_.update(ol.extent.boundingExtent(segmentData.segment),
+ segmentData);
+ }
}
return false;
};
@@ -599,23 +613,8 @@ ol.interaction.Modify.prototype.insertVertex_ = function(segmentData, vertex) {
var rTree = this.rBush_;
goog.asserts.assert(goog.isDef(segment));
rTree.remove(segmentData);
- var uid = goog.getUid(feature);
- var segmentDataMatches = [];
- this.rBush_.forEachInExtent(geometry.getExtent(),
- function(segmentData) {
- if (goog.getUid(segmentData.feature) === uid) {
- segmentDataMatches.push(segmentData);
- }
- });
- for (var i = 0, ii = segmentDataMatches.length; i < ii; ++i) {
- var segmentDataMatch = segmentDataMatches[i];
- if (segmentDataMatch.geometry === geometry &&
- (!goog.isDef(depth) ||
- goog.array.equals(segmentDataMatch.depth, depth)) &&
- segmentDataMatch.index > index) {
- ++segmentDataMatch.index;
- }
- }
+ goog.asserts.assert(goog.isDef(index));
+ this.updateSegmentIndices_(geometry, index, depth, 1);
var newSegmentData = /** @type {ol.interaction.SegmentDataType} */ ({
segment: [segment[0], vertex],
feature: feature,
@@ -638,3 +637,114 @@ ol.interaction.Modify.prototype.insertVertex_ = function(segmentData, vertex) {
newSegmentData2);
this.dragSegments_.push([newSegmentData2, 0]);
};
+
+
+/**
+ * Removes a vertex from all matching features.
+ * @private
+ */
+ol.interaction.Modify.prototype.removeVertex_ = function() {
+ var dragSegments = this.dragSegments_;
+ var segmentsByFeature = {};
+ var component, coordinates, deleted, dragSegment, geometry, i, index, left;
+ var newIndex, newSegment, right, segmentData, uid;
+ for (i = dragSegments.length - 1; i >= 0; --i) {
+ dragSegment = dragSegments[i];
+ segmentData = dragSegment[0];
+ geometry = segmentData.geometry;
+ coordinates = geometry.getCoordinates();
+ uid = goog.getUid(segmentData.feature);
+ left = right = index = undefined;
+ if (dragSegment[1] === 0) {
+ right = segmentData;
+ index = segmentData.index;
+ } else if (dragSegment[1] == 1) {
+ left = segmentData;
+ index = segmentData.index + 1;
+ }
+ if (!(uid in segmentsByFeature)) {
+ segmentsByFeature[uid] = [left, right, index];
+ }
+ newSegment = segmentsByFeature[uid];
+ if (goog.isDef(left)) {
+ newSegment[0] = left;
+ }
+ if (goog.isDef(right)) {
+ newSegment[1] = right;
+ }
+ if (goog.isDef(newSegment[0]) && goog.isDef(newSegment[1])) {
+ component = coordinates;
+ deleted = false;
+ newIndex = index - 1;
+ switch (geometry.getType()) {
+ case ol.geom.GeometryType.MULTI_LINE_STRING:
+ coordinates[segmentData.depth[0]].splice(index, 1);
+ deleted = true;
+ break;
+ case ol.geom.GeometryType.LINE_STRING:
+ coordinates.splice(index, 1);
+ deleted = true;
+ break;
+ case ol.geom.GeometryType.MULTI_POLYGON:
+ component = component[segmentData.depth[1]];
+ /* falls through */
+ case ol.geom.GeometryType.POLYGON:
+ component = component[segmentData.depth[0]];
+ if (component.length > 4) {
+ if (index == component.length - 1) {
+ index = 0;
+ }
+ component.splice(index, 1);
+ deleted = true;
+ if (index === 0) {
+ // close the ring again
+ component.pop();
+ component.push(component[0]);
+ newIndex = component.length - 1;
+ }
+ }
+ break;
+ }
+
+ if (deleted) {
+ this.rBush_.remove(newSegment[0]);
+ this.rBush_.remove(newSegment[1]);
+ geometry.setCoordinates(coordinates);
+ goog.asserts.assert(newIndex >= 0);
+ var newSegmentData = /** @type {ol.interaction.SegmentDataType} */ ({
+ depth: segmentData.depth,
+ feature: segmentData.feature,
+ geometry: segmentData.geometry,
+ index: newIndex,
+ segment: [newSegment[0].segment[0], newSegment[1].segment[1]]
+ });
+ this.rBush_.insert(ol.extent.boundingExtent(newSegmentData.segment),
+ newSegmentData);
+ this.updateSegmentIndices_(geometry, index, segmentData.depth, -1);
+
+ this.overlay_.removeFeature(this.vertexFeature_);
+ this.vertexFeature_ = null;
+ }
+ }
+ }
+};
+
+
+/**
+ * @param {ol.geom.SimpleGeometry} geometry Geometry.
+ * @param {number} index Index.
+ * @param {Array.<number>|undefined} depth Depth.
+ * @param {number} delta Delta (1 or -1).
+ * @private
+ */
+ol.interaction.Modify.prototype.updateSegmentIndices_ = function(
+ geometry, index, depth, delta) {
+ this.rBush_.forEachInExtent(geometry.getExtent(), function(segmentDataMatch) {
+ if (segmentDataMatch.geometry === geometry &&
+ (!goog.isDef(depth) ||
+ goog.array.equals(segmentDataMatch.depth, depth)) &&
+ segmentDataMatch.index > index) {
+ segmentDataMatch.index += delta;
+ }
+ });
+};
Something went wrong with that request. Please try again.