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

Open
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?

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.

4 participants