Added segment snapping to the ShapeEditor #132

mathijs-dumon · 2018-12-14T11:09:57Z

This is an addition I had to make I think you might want to merge into the main project

paulmeems-waterwatchfoundation · 2018-12-14T12:08:05Z

Thanks for the pull request. We will have a look and merge it soon.
If you're interested I could add you to the team and you can help improve MapWinGIS even more.

mathijs-dumon · 2018-12-14T12:20:20Z

That'd be awesome actually, as I'm also preparing a pull request for MapWindow5 to incorporate this.

jerryfaust · 2019-01-01T00:12:12Z

src/ComHelpers/ShapefileHelper.cpp

+	{
+		VARIANT_BOOL visible;
+		sf->get_ShapeVisible(id, &visible);
+		if (!visible) continue;


It's best not to treat a VARIANT_BOOL as a bool. Since VARIANT_TRUE is a short integer (-1), and bool is an intrinsic type, you may not always get the expected result. It would be better to say something like
if (visible == VARIANT_FALSE) continue;

This may seem a trivial point, but we've had issues in the past when mixing these types.

I'm pretty new to C and C++, most of the changes were made by looking at other code and copying what I need, thanks for the explanation. Can you point me to a good reference for this in particular?

Here's a pretty good explanation, noting particularly to beware of comparing the various 'true' values:

BOOL vs. VARIANT_BOOL vs. BOOLEAN vs. bool

I should add that being a COM interface, our API needs to use VARIANT_BOOL. However, internally, it is often beneficial to use bool since it is more concise and straightforward. Only convert back to VARIANT_BOOL as necessary.

jerryfaust · 2019-01-01T00:22:18Z

src/ComHelpers/ShapefileHelper.cpp

+// *****************************************************************
+bool ShapefileHelper::GetClosestPointOnSegment(double ax, double ay, double bx, double by, double px, double py,
+	double *rx, double *ry) {
+


It would be helpful to document the variables and intent of this method.

It's not clear why this is a boolean function since it always returns true and your not using the result. If it can actually fail to get the closest point on the segment, then we should trap that case and return false.

jerryfaust · 2019-01-01T00:32:14Z

src/Control/Map_Edit.cpp

+
+	double projX, projY;
+	bool snapped = FindSnapPointCore(x, y, &projX, &projY);
+	if (!snapped) {


Similar to earlier, FindSnapPointCore returns a VARIANT_BOOL, and you're setting it into a bool variable. Either 'snapped' should be changed to VARIANT_BOOL, OR perhaps instead, we should change FindSnapPointCore to return a bool, since it is an internal function and is more easily handled with the standard bool data type.

jerryfaust · 2019-01-01T00:44:03Z

src/Control/Map_Edit.cpp

-		this->PixelToProjection(x, y, projX, projY);

 		// highlighting of vertices
 		if (behavior == ebVertexEditor)


I have to look further into this - 'behavior' was originally defined (within this block of code) as tkEditorBehavior, but you have redefined it (above) as tkLayerSelection, and assigned the result of get_SnapBehavior. It seems that this test (behavior == ebVertexEditor) may not be giving back the expected result.

jerryfaust

Hello @mathijs-dumon

First, I should thank you for submitting this. It will be a nice feature. Additionally, we may be able to optimize the search for nearest snap point by using a GEOS library nearest-point method.

I've done an initial code review and added some comments. I have not yet tested any code, and I will do that soon. You do not need to revise anything; I just wanted to let you see my initial review in case you have any replies to my comments. My next step will be to build the code and probably make some of the suggested changes. This is the first Pull Request that I am reviewing, so the process is new to me.

Regards.
Jerry.

mathijs-dumon · 2019-01-01T08:25:29Z

Hey @jerryfaust,

Thanks for the first review. I'm not familiar with GEOS at all. I did check the QGis implementation which also seemed to use a custom method for it. Performance wise this seemed ok, since the features (to be snapped to) are clipped first to reduce their number. But this is based on limited testing.

If you want me to help out, just ask!

Mathijs

jerryfaust · 2019-01-01T21:39:14Z

We use GEOS internally (as does GDAL) for various geospatial operations (e.g. intersection, containment, etc). One thing we're working toward is consistency, so that all of our geospatial operations are done using the same GEOS functions, rather than some of them using GEOS and others using our own algorithms. That's the reasoning behind MWGIS-136. In this way, we will always get consistent results, and we can let the GEOS people focus on optimized algorithms.

So you did the right thing by narrowing down the number of shapes to check for snapping, but I think we can go another step further, replacing the algorithm for finding the nearest snap point on each shape (GetClosestSnapPosition), using a function within the GEOS library.

jerryfaust · 2019-01-04T06:54:18Z

@mathijs-dumon
Are you able to make some of the suggested changes, and update/resubmit the Pull Request? If you could, and retest on your end, it would be helpful. If you are not able to within the next couple days, let me know, and I will go ahead and make the changes, and ask you to retest after the fact.
Regards.
Jerry.

mathijs-dumon · 2019-01-07T21:02:59Z

Hey Jerry,
was busy figuring out something else past few days (as you'll probably see).
I'll try to find some time this week for this if that's ok?

cheers & happy new year,
Mathijs

mathijs-dumon · 2019-01-24T13:22:37Z

@jerryfaust I've made some changes incorporating most of what you suggested and rebased the branch - can you double check I did not forget anything?

jerryfaust · 2019-01-25T06:10:31Z

Thanks, @mathijs-dumon, I'll hopefully get to it this weekend.

jerryfaust · 2019-01-27T08:40:45Z

Hello @mathijs-dumon.

I've committed some follow-up changes to the Pull request; some related to the use of VARIANT_BOOL, and take note particularly of the change to HandleOnMouseMoveShapeEditor in file Map_Edit.cpp. The 'behavior' variable was being used for two separate purposes, first as tkLayerSelection, then as tkEditorBehavior, and the second case without being initialized. Please review to make sure that it is accomplishing the intended purpose. I have not been able to try it out yet, but we can both spend more time with it before the next release.

Thanks again for the contribution.
Regards,
Jerry.

mathijs-dumon mentioned this pull request Dec 14, 2018

Adding segment/vertex snapping options to the shape editor MapWindow/MapWindow5#39

Closed

jerryfaust reviewed Jan 1, 2019

View reviewed changes

mathijs-dumon added 3 commits January 24, 2019 14:19

Added snapping modes: to segments, to vertices or both

95ee8e7

Bugfixes to line snapping support

1f95f01

Changed segment snapping - using only mapwingis/GEOS methods

06c9ade

mathijs-dumon force-pushed the develop-snapping branch from 143ee96 to 06c9ade Compare January 24, 2019 13:21

jerryfaust merged commit e4abdbf into MapWindow:develop Jan 27, 2019

jerryfaust added a commit that referenced this pull request Jan 27, 2019

Follow-up to pull request #132; minor adjustments.

e6086cb

Added segment snapping to the ShapeEditor #132

Added segment snapping to the ShapeEditor #132

Uh oh!

Conversation

mathijs-dumon commented Dec 14, 2018

Uh oh!

paulmeems-waterwatchfoundation commented Dec 14, 2018

Uh oh!

mathijs-dumon commented Dec 14, 2018

Uh oh!

jerryfaust Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathijs-dumon Jan 1, 2019

Choose a reason for hiding this comment

Uh oh!

jerryfaust Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryfaust Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryfaust Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryfaust Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryfaust left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathijs-dumon commented Jan 1, 2019

Uh oh!

jerryfaust commented Jan 1, 2019

Uh oh!

jerryfaust commented Jan 4, 2019

Uh oh!

mathijs-dumon commented Jan 7, 2019

Uh oh!

mathijs-dumon commented Jan 24, 2019

Uh oh!

jerryfaust commented Jan 25, 2019

Uh oh!

jerryfaust commented Jan 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jerryfaust Jan 1, 2019 •

edited

Loading

jerryfaust Jan 1, 2019 •

edited

Loading

jerryfaust Jan 1, 2019 •

edited

Loading

jerryfaust Jan 1, 2019 •

edited

Loading

jerryfaust Jan 1, 2019 •

edited

Loading

jerryfaust left a comment •

edited

Loading