Modify interaction sometimes does not allow to create a new vertex #1807

Merged
merged 4 commits into from Mar 12, 2014

Projects

None yet

4 participants

@ahocevar
Member
ahocevar commented Mar 5, 2014

When on a segment between two existing vertices, it is sometimes impossible to drag the virtual vertex to create a new one. This can easily be reproduced in the modify-features examples.

@ahocevar
Member
ahocevar commented Mar 5, 2014

I am working on this now.

@tschaub
Member
tschaub commented Mar 5, 2014

@ahocevar should we ticket separately the issue about creating vertices in the wrong locations? Or do you anticipate this will solve that?

@ahocevar
Member
ahocevar commented Mar 5, 2014

Pull request with fix attached. Thanks for any review.

@tschaub
Member
tschaub commented Mar 5, 2014

As an alternative to introducing a tolerance, we should be able to determine if this point intersects the segment since we generated the point. I'd be curious what the code below generates for the points you're seeing fail:

ol.coordinate.equals(vertex, ol.coordinate.closestOnSegment(vertex, segment));
@ahocevar
Member
ahocevar commented Mar 5, 2014

Unfortunately things are not that straightforward:

> var segment = [[1e8+123456, 1e6+123456], [1e8+654321, 1e6-654321]];
undefined
> ol.coordinate.closestOnSegment([1e8, 1e6], segment);
[100141704.82843556, 1096719.4115375485]

> ol.coordinate.closestOnSegment([100141704.82843556, 1096719.4115375485], segment);
[100141704.82843556, 1096719.4115375476]

> ol.coordinate.closestOnSegment([100141704.82843556, 1096719.4115375476], segment);
[100141704.82843556, 1096719.4115375471]

In this example, it takes 5 more iterations for the closest point to stabilize.

@ahocevar
Member
ahocevar commented Mar 6, 2014

I found a much smarter solution now, without the need to measure the distance of the vertex to the segment in #handleDragStart(). Thanks for another review.

@ahocevar
Member
ahocevar commented Mar 6, 2014

@tschaub Does bf1be62 address the concern you brought up in today's hangout? I think it makes sense to only treat segments as shared when they have the exact same vertex coordinates, which should be the case for topology based data sets.

@tschaub
Member
tschaub commented Mar 6, 2014

@ahocevar actually, my concern was the opposite :)

    |
  A | B
----X----
    C

The question is what to do if point X is shared by edges of polygons A and B but not C. If the user clicks on/near X and drags, we can:

  • insert X into edge of C and drag everything together, or
  • drag X for A and B and leave C alone

I think the first would be more desirable. Given that people can control which features are involved in shared editing by selecting them, if they don't want this, they don't need to select C.

@ahocevar
Member
ahocevar commented Mar 6, 2014

@tschaub This raises more questions. Take a look:

    |   |   |
  A | B | D |
----X---Y---Z
    C

Here Y is shared by B and D, but not C. Z is shared by C and D. Now if we insert X into the edge of C and drag everything together, then the segment Y-Z will no longer align with the segment X-Z. Expected or not?

@tschaub
Member
tschaub commented Mar 6, 2014

@ahocevar good point. Can o' worms. I think starting to test on real data with your changes here makes sense.

@ahocevar
Member
ahocevar commented Mar 6, 2014

I did test with the countries file that is also used in the vector-layer
example, and I did not encounter any flaws.
On 6 Mar 2014 19:42, "Tim Schaub" notifications@github.com wrote:

@ahocevar https://github.com/ahocevar good point. Can o' worms. I think
starting to test on real data with your changes here makes sense.

Reply to this email directly or view it on GitHubhttps://github.com/openlayers/ol3/pull/1807#issuecomment-36921196
.

@ahocevar
Member
ahocevar commented Mar 6, 2014

@tschaub I updated the example so you can now choose whether to edit test data or real data. Feel free to try it out live at http://ahocevar.net/ol3/float-no-zero/examples/modify-features.html.

@marcjansen
Member

@ahocevar I am unable to add a vertex at the southernmost triangle-feature in the "Test data" layer. When I select that polygon and hover the edges, only for the / and the \ edge, a draggable point appears, not for the _ side. Am I doing sth. wrong? (Tested on Chrome 27.0.1453.93, Linux).

@marcjansen
Member

@tonio Yeah, that's a possible reason. IMHO we should also adress this.

@marcjansen
Member

#1817 fixes the unclosed polygon, thanks for the pointer @tonio.

ahocevar added some commits Mar 6, 2014
@ahocevar ahocevar Keep track of intersecting segments
Because we have nodes sorted by segment distance from the
editing vertex in #handleMouseAtPixel(), it is cheap to create a
hash of intersecting segments there. Now in #handleDragStart(),
we do not need to measure the distance of the vertex to the
segment. Instead, we just test if the segment is in the hash.
4293540
@ahocevar ahocevar Added another linestring to show shared segment editing 97f8fdb
@ahocevar ahocevar Limit shared segment editing
To avoid surprises, we enable shared segment editing only on
segments that have the same vertex coordinates.
bc79b89
@ahocevar ahocevar Add a layer with real data to the example 6fbafef
@ahocevar
Member
ahocevar commented Mar 7, 2014

Thanks @marcjansen, branch and live example updated.

@ahocevar
Member

@tschaub, is this good to merge now?

@tschaub
Member
tschaub commented Mar 12, 2014

Looks like a great fix @ahocevar. Please merge.

I think the modify feature example is much nicer with "real" data - so I appreciate you adding that. I think it adds a bit of unnecessary complexity to the example to have the data chooser, and I'd be inclined to make a separate example for testing the variety of geometry types in the mock data (or better yet, more tests and more interesting examples for people to play with). But that can be handled separately if you want to merge as is.

@ahocevar
Member

Ok, I'll create a geojson file with the test data and modify the example to use the countries file by default, with a comment that points to the test data file. That should make the example easier to read.

@ahocevar
Member

Hm. I think I'll handle the example split (or better: addition of unit tests) separately, because the style arrays could also use some love to make them simpler and easier to read.

@ahocevar ahocevar merged commit 1fbdf47 into openlayers:master Mar 12, 2014

1 check passed

default The Travis CI build passed
Details
@ahocevar ahocevar deleted the ahocevar:float-no-zero branch Mar 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment