Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new Measure Radius tool to measure map tools #60015

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benwirf
Copy link
Contributor

@benwirf benwirf commented Dec 26, 2024

Description

Based on feedback, I am going to rework this proposal in the coming days.

This PR would add a new Measure Radius map tool.

Although it is now closed, this does address the original request of #55523 which was a circular radius measuring tool in core.

Initially, I started writing new map tool & dialog classes, but quickly realized it would be better and more efficient to re-use the existing map tool & dialog. Hence, the QgsMeasureTool class is modified to include an additional measureRadius boolean parameter in the constructor. The qgsmeasurebase.ui is not touched, but is modified in qgsmeasuredialog.cpp to hide the table and add line edits showing the x & y coordinates of the center & exterior points defining the radius. The existing point and line rubber bands are also re-used, with a circular polygon rubber band added which is set to a polygon geometry created via the QgsCircle class. The general behavior is that a left click starts measuring, left click while measuring resets the rubber bands & restarts measuring; right click stops measuring and keeps the rubber bands visible until user left-clicks again, hits escape to restart, clicks New button on dialog, closes the dialog or changes tool.

measure-radius-4

I've added a QgsColorButton in the Measure Tool section of the map tools options dialog, enabling the user to change the circular rubber band color. The selected color is stored and retrieved via a QgsSettingsEntryColor in QgsSettingsRegistryCore (maybe it would be better in QgsSettingsRegistryGui?)

Screenshot from 2024-12-26 15-57-15

measure-radius-2

Some additional screenshots/screencasts below:

Modified dialog for Measure Radius tool.
Screenshot from 2024-12-26 16-49-48

Measure Radius icon.
Screenshot from 2024-12-26 16-50-43

Changing project CRS.
measure-radius-3

Please note: I still need to update the tests for QgsMeasureTool but it's taken me a bit to get to this point so I'm keen to just test the waters and get some feedback first.

@nyalldawson, I would certainly appreciate your opinion here (is this any good & a worthwhile addition to core?). Also- should I create a QEP as per: qgis/QGIS-Enhancement-Proposals#310?

@github-actions github-actions bot added this to the 3.42.0 milestone Dec 26, 2024
Copy link

github-actions bot commented Dec 26, 2024

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit d27654a)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit d27654a)

@agiudiceandrea
Copy link
Contributor

agiudiceandrea commented Dec 26, 2024

Thanks @benwirf!
Some notes, not to be considered a proper review:

  • since the functionality seems to me similar to the "Measure Line" tool, couldn't it be added to the very "Measure Line" tool?
  • does the tool take care of the fact that the locus of points equidistant from a given point is usually not a circle for almost all the representation of the ellipsoid surface on a plane?

@benwirf
Copy link
Contributor Author

benwirf commented Dec 26, 2024

Hi @agiudiceandrea, thanks for the feedback. On the first point, do you mean adding a circular rubber band to the existing Measure Distance tool? I suppose it could be added there, but since the measure distance tool can consist of multiple consecutive line segments, would you envisage the rubber band be added to each segment as it is being drawn? or the first segment only? I'm not sure if that would fit with the multi-segment/ path drawing functionality and I feel that it could be annoying.
I certainly take your second point on board, and I share your misgivings about that. The method which creates the circular geometry looks like:

QgsGeometry QgsMeasureTool::createCircleGeom()
{
  QgsPoint tmpCntrPt = QgsPoint( mPoints.at( 0 ) );
  QgsPoint tmpOutrPt = mRubberBand->asGeometry().vertexAt( 1 );
  double dist = mRubberBand->asGeometry().length();
  double az = tmpCntrPt.azimuth( tmpOutrPt );
  QgsCircle circ = QgsCircle( tmpCntrPt, dist, az );
  QgsPolygon *poly = circ.toPolygon( 360 );
  QgsGeometry circleGeom = QgsGeometry( poly );

  return circleGeom;
}

By some basic tests/ comparisons, the results seem equivalent to buffering points in various CRS's and would be subject to the same inaccuracies. Results should be fairly accurate in local projected CRS and less so (to outrageously wrong) in other (global) CRS's e.g. web mercator (which should not be used for distance calculations anyway). And obviously results would be worse over larger distances and further from the equator in those distorted CRSs. To an extent, I think the same issues would affect the other measuring tools. Therefore, I think there is still usefulness to this tool as a complement to the others (unless my whole idea here is flawed)! If you can suggest a better approach I'm all ears :-)

@agiudiceandrea
Copy link
Contributor

agiudiceandrea commented Dec 26, 2024

do you mean adding a circular rubber band to the existing Measure Distance tool? I suppose it could be added there, but since the measure distance tool can consist of multiple consecutive line segments, would you envisage the rubber band be added to each segment as it is being drawn?

Yes, I mean I think the functionality may be added to the "Measure Line" tool for the latest segment being drawn, with an option to enable or disable it (default disabled).

To an extent, I think the same issues would affect the other measuring tools.

When the ellipsoidal measurement mode is set, IMHO it shouldn't be correct to always draw a circle as the locus of points equidistant from the start point (which the "circular" rubber band should represent). Otherwise the user may be misled to think the all the points on that circle's circumference are at the same ellipsoidal distance for the start point, while it is not generally the case.

@nyalldawson
Copy link
Collaborator

A couple of general thoughts (I can't review code till February, I'm away from my PC till then):

  • I share @agiudiceandrea concern about correctly handling ellipsoidal measurements in the results. All the other measure tools default to ellipsoidal calculations (regardless of CRS) and it's extremely important this tool will tool, or we risk giving users misleading results. Maybe there's methods from proj's API or geographiclib which can be used to calculate the geometries correctly.
  • I think a seperate tool is fine, I'd rather not see the other measure tool cluttered with options (the only UI clutter added by a new tool is an extra action in a drop down menu)
  • I don't like a specific option for the rubber band colour for just this tool. It should just use the same rubber band appearance as the existing measure area tool uses.

@benwirf
Copy link
Contributor Author

benwirf commented Dec 27, 2024

Hi @nyalldawson, thank you for the feedback. I will remove the rubber band color option! I will also look at implementations of geodesic buffering to create the rubber band geometry when 'ellipsoidal' is checked. I will be offline myself over the weekend, but will do some more work on this next week.

@nyalldawson
Copy link
Collaborator

@benwirf thinking more about it, you probably want to use the approach outlined in https://gis.stackexchange.com/questions/121489/1km-circles-around-lat-long-points-in-many-places-in-world/121539#121539 . Wrap it up nicely in a function in QgsDistanceArea which takes a point centre argument, radius and "number of vertices"/densification argument and you've got a winner 👍

@uclaros
Copy link
Contributor

uclaros commented Dec 27, 2024

I would expect a Measure Radius tool to be what is described in #59969
As this practically just measures a distance, I'm also pro to incorporate it within the measure distance tool. Circle would be drawn for the last measure segment (getting the shape correct for ellipsoid calculations is absolutely crucial for me too).
In order to avoid cluttering the tool's ui, this could be enabled by a checkbox in the measure tool options.

@benwirf
Copy link
Contributor Author

benwirf commented Dec 27, 2024

Thank you all for the feedback. I will certainly do some major reworking of this PR in the next week. @nyalldawson, thanks for the GISSE link. I was also doing some digging and found some potential inspiration here: Approximating Geodesic Buffers with PyQGIS, which seems a similar approach i.e. using a custom azimuthal equidistant projection centered on the point.
@uclaros, I will change the terminology and not use "Measure Radius" to describe this. In the case that this would be added to the existing Measure Distance tool, perhaps the checkbox could say "Show buffer area" or something similar. I would also be interesting in working on a measure curve radius tool as described in #59969, but that would be a separate tool/action in the measure tools menu, right?

@benwirf
Copy link
Contributor Author

benwirf commented Jan 5, 2025

Updating here with some progress @agiudiceandrea, @nyalldawson, @uclaros:

I have added this functionality to the existing measure distance tool, with a check box in the measure tool section of the map tool options dialog and used the same rubber band color as the other rubber bands (rather than adding another rubber band color option).

Screenshot from 2025-01-01

I've also implemented the ellipsoidal buffer calculation by buffering in an azimuthal equidistant projection centered on the 2nd last line rubber band vertex then transforming back to the canvas CRS.

buffer_rb-2

This mostly works well except when the transformed geometry crosses the antimeridian or (I think) is outside the bounds of the canvas CRS, then the returned geometry is invalid, leading to incorrect results. I have managed to get wrapping the geometry around the antimeridian partly working, using an approach similar to that outlined here:

https://gis.stackexchange.com/questions/396460/qgis-vector-layer-stretched-across-entire-map/396562#396562

but this still fails when the geometry is outside the latitudinal bounds of the canvas CRS.

buffer_rb-1

Similar to this issue: satellogic/telluric#22

My plan is to dig into other libraries e.g. Proj/ Cartopy for ideas but my annual leave is ending and I have to go back to my regular job tomorrow, so this will be weekend project for the foreseeable future.

So just wondering if anyone has any clever ideas or suggested workflows for dealing with this?

@@ -77,29 +78,54 @@ QgsMeasureDialog::QgsMeasureDialog( QgsMeasureTool *tool, Qt::WindowFlags f )
mTable->addAction( copyAction );
}

if ( mMeasureRadius )
{
// Hide the table, add line edits for centre & exterior point and rename 'Total' label to 'Radius'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Hide the table, add line edits for centre & exterior point and rename 'Total' label to 'Radius'
// Hide the table, add line edits for center and exterior point and rename 'Total' label to 'Radius'

editTotal->setText( formatDistance( mTotal + d, mConvertToDisplayUnits ) );
}
// If measuring radius, the table is hidden so just set the total, exterior x & y and return
else if ( mMeasureRadius )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
else if ( mMeasureRadius )
else

@@ -69,6 +69,12 @@ class APP_EXPORT QgsMeasureDialog : public QDialog, private Ui::QgsMeasureBase
//! When any external settings change
void updateSettings();

//! Show the center point X & Y values in line edits when measuring radius
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! Show the center point X & Y values in line edits when measuring radius
//! Show the center point X and Y values in line edits when measuring radius

Everywhere, replace '&' by 'and'

QgsPoint tmpCntrPt = QgsPoint( mPoints.at( 0 ) );
QgsPoint tmpOutrPt = mRubberBand->asGeometry().vertexAt( 1 );
double dist = mRubberBand->asGeometry().length();
double az = tmpCntrPt.azimuth( tmpOutrPt );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is azimuth really expected? By default, Z will have the same default value. Maybe you can snap, but it's never shown in the dialog. I doubt that you expect to have a particular oriented circle here.

QgsPoint tmpOutrPt = mRubberBand->asGeometry().vertexAt( 1 );
double dist = mRubberBand->asGeometry().length();
double az = tmpCntrPt.azimuth( tmpOutrPt );
QgsCircle circ = QgsCircle( tmpCntrPt, dist, az );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to not use the QgsCircle::from2Points method?

double dist = mRubberBand->asGeometry().length();
double az = tmpCntrPt.azimuth( tmpOutrPt );
QgsCircle circ = QgsCircle( tmpCntrPt, dist, az );
QgsPolygon *poly = circ.toPolygon( 360 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
QgsPolygon *poly = circ.toPolygon( 360 );
QgsPolygon *poly = circ.toPolygon( 360 );

Why 360? FYI, it's not degree, but the number of segment. By default, it's 36, why? More than 8 years later, I've just realized that I made a mistake... It's 8 segments (as for a buffer) per quadrant, i.e. 4 x 8 ... 36... “We leave the verification as an exercise for the reader” 😅

However, I understand it's tempting to have more segments to get a more segmented disk that really looks like a disk rather than a regular polygon. I just want to ensure a good understanding of the function and a good compromise between the number of segments, performance, and visualization.

From what I remember, 32/36 segments is usually sufficient for < 1 km. Here, indeed, we might be dealing with larger measurements. In the absence of a formula, which we can add later, I think 64 segments should be sufficient. It's up to you to tell me based on what you can observe. We need to find the right compromise.

I'll look into finding a formula on my side, perhaps taking inspiration from the sagitta deviation to determine the optimal number of segments for a given radius.

@benwirf benwirf marked this pull request as draft January 12, 2025 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants