[advanced digitizing] Add circles intersection digitizing tool

[nirvn]

Description

This PR adds a brand new advanced digitizing tool allowing for users to pick/digitize a point at the intersection of two circles. In action:

vokoscreenNG-2024-06-04_11-02-23.mp4

The implementation adds an abstract advanced digitizing tool class to allow for more such "COGO" tools to be added in the future (e.g. intersection between a circle and a line + bearing).

The new functionality is exposed to our python bindings, which also means people can write plugins and prototype additional functionalities in python.

(The PR temporarily includes the construction guides work, I'll rebase when that is merged)

[github-actions]

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 99c1c1d)

[uclaros]

While I agree that this functionality is missed in QGIS, the UX of this implementation is not really improved over the previous attempt #45904

Currently, a single circle constraint exists by using the Distance constraint, and imho we should take advantage of that and use it at least for one of the two intersecting circles.

How about we add the distance circles as guides too, then allow snapping on their intersections?

[lbartoletti]

I'm +1 to have this kind of tool in QGIS, but we were refused this one because we were adding a CoGO toolbar (later, I think there were other toolbars, but hey ...).

We finally made a plugin for it, with other CoGo tools that I didn't release because we moved on to other subjects and the spirit of the project at the time no longer suited me or my colleague... Tired of the feeling of differences in treatment, we didn't open the QEP, since we felt that certain more important changes were often made without this "precious discussion"...

Nevertheless, I'll support you @nirvn so that we can finally have this kind of tool in QGIS! And I'm willing to come back and support you. Let's see how we can mutualize this with a UI/UX that suits us and our use cases.

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[github-actions]

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

[nirvn]

Good reminder bot, I'll push this forward over the weekend.

[nirvn]

@uclaros , hey, sorry for taking so long to engage with your comment, life and priorities got in the way :)

While I appreciate what you are suggesting here, I do not think a COGO tool should rely on construction guides to work. Very often, these operations are one off. Most basic case here: I'm back from my field survey where I took accurate reading of a pair of reachable points and distances to map out the intersecting point. I've got no need here for construction guides as these two circles will never be re-used.

So IMHO, I would argue here that COGO operations, very often ephemeral, should be kept separate from construction guides.

What I very much do like in your suggestion is adding the possibility to add circular construction guides when the distance is looked during construction mode. That's something we should consider in a separate PR.

[nirvn]

@uclaros , @lbartoletti , I do feel this is a UI improvement from #45904 in quite a few fronts here:

1/ the big difference here is that it integrates with the CAD tool and ends up adding a vertex to any map tool that shares the CAD tool class. This means support for adding a vertex using this tool while digitizing line or a polygon (think about the need to trace a plot of land that includes circle-intersection points). The previous PR was a separate process that did not plug into the existing digitizing framework, only worked for point geometry layers, and didn't play along other map tools (e.g. annotations).

2/ the UI does not involve adding a new toolbar in our main windows but rather add to the CAD panel, which IMHO gives it a better location while avoiding cluttering the main window toolbar area.

@lbartoletti , sorry you were left frustrated by the process and glad to read I've got your support on this one. On the plugin side of things, the PR allows for other tools to be prototyped and registered via plugin. If your unreleased COGO plugin included other tools, I'd love to see if it's possible to port them to this framework. It'd make for a great QGIS 3.40 plugin :)

[nirvn]

A screencast of what I was explaining above in terms of integration with map tools:

vokoscreenNG-2024-07-28_16-32-25.mp4

Here, I was not able to get the top-right corner of the rice field (too muddy), so I had to take two points + distances. The video demonstrates how I could nevertheless go back to the office and create the plot polygon by using the COGO tool to add the 4th vertex.

[nirvn]

@nyalldawson , I've (partially) addressed the comment you made in #45904 by adding a registry of advanced digitizing tools (that'll be useful to you @lbartoletti ) and transforming the toolbar action into a drop down menu that will contain all the registered tools.

ATM, I think I'm -1 on doing the gimp-like subpanel because a/ we have only one tool, and more importantly b/ these tools can still rely on the CAD constraint widgets, so opening a subpanel would just interfered with that.

I do think the toolbar drop down menu does embrace the spirit at least of a tools container :)

[nyalldawson]

@nirvn Ok, i've done a review based on the code as is. I'm fine with the new GUI approach here too, we can always revisit the tool box concept in future if/when needed.

Keen to hear @uclaros / @lbartoletti 's thoughts on the new approach too!

[uclaros]

@nirvn Thanks for being open to discussion!
I agree with all your points above and the UI is improved (cad tools integration was a must) however the UX is similar.: the user still needs to do lots of clicks to get what he wants. Your screencasts display a slightly awkward workflow: point, point, distance, distance while I think the UI should be optimized for: point, distance, point, distance.

I'd be very happy with the resulting UX if the following were tuned:

1. QgsAdvancedDigitizingCirclesIntersectionTool::createWidget() should start with mCircle1Digitize->setChecked( true ); so that the user can directly pick his first point once the tool is activated.
2. Once a point is picked, mCircle1Distance / mCircle2Distance should gain focus respectively
3. Once mCircle1Distance is accepted (enter pressed), mCircle2Digitize should be triggered
4. At the beginning of QgsAdvancedDigitizingCirclesIntersectionTool::createWidget(), we could check mCadDockWidget->constraintDistance() and if one is set, pre-populate mCircle1... widgets, and continue with having mCircle2Digitize->setChecked( true ); so the user can directly pick his second point
5. If the project CRS is not projected, the tool should probably be disabled
6. It would be great if we could have a bindable shortcut for activating the tool.

[lbartoletti]

@nirvn Thanks.

I'm not able to review and test this PR at the moment. But, be sure that, I'll be happy to review and test at the end of August.

[nirvn]

@uclaros , thanks for the feedback. I've implemented the UX logic you've described (point 1 through to 4).

With regards to project CRS, I'll think of a way to do that in a follow up PR (I'm thinking of a tool's flags() that would return whether it supports geographic coordinates.

@lbartoletti , OK, no rush. I will merge this PR now as I think we're all in agreement with the foundation here, and address your review in a month in follow up PRs. Glad we're moving forward on this 🙏

[uclaros]

Thanks @nirvn for handling my suggestion. However I do not see how 4 is implemented, am I missing something?
This is important imho as it allows for first circle to be placed at the last digitized segment, which is probably the most common use case.

Regarding 3, we should probably first check if point 1 has been set, or the user might be trying to do distance, point, distance, point

I didn't get a chance to build and test yet, but shouldn't mCircle1Digitize and mCircle2Digitize buttons be mutually exclusive?

[nirvn]

@uclaros , heh, I had meant 1-3, but then you insisting on 4 led to it being implemented ;)

Also, I've made the digitize toggles mutually exclusive, good call. As for the rest, let's tweak things in follow up PRs. I'm not sure we should overthink the logic too much here though.

That said, big thanks for the feedback, it made the feature much better 🙏

[qgis-bot]

@nirvn
This pull request has been tagged as requiring documentation.

A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged.

Please update the description (not the comments) with helpful description and screenshot to help the work from documentors.
Also, any commit having [needs-doc] or [Needs Documentation] in will see its message pushed to the issue, so please be as verbose as you can.

Thank you!

[qgis-bot]

@nirvn
A documentation ticket has been opened at qgis/QGIS-Documentation#9184
It is your responsibility to visit this ticket and add as much detail as possible for the documentation team to correctly document this change.
Thank you!

[uclaros]

@nirvn I would have appreciated it if you waited for the review process to complete before merging this!

A couple other issues that need to be resolved before I'd stop considering this half-baked:

• Handle esc button: users are trapped in this mode and cannot abort unless they do add a point.
• Handle right click: right click should do the same as esc, right now it forwards the right click to the map tool
• Once a center is placed and focus goes into its radius widget, mouse move should track the distance from that center and update the spinbox value, similar to how d works for advanced digitizing panel.

[nirvn]

@uclaros , good catch regarding the ESC and right-click handling, I've addressed these alongside the label renaming suggestions you dropped in this follow up PR: #58289

As for merging this to begin with, after having answered to reviews and implemented UI/UX suggestions from you and others over the last 7 weeks, without an explicit request for further review I think it was fair to merge to get as much testing as we can. On a friendly note, I'm not sure labeling efforts shown this PR as "half-baked" is a constructive way to go about it nor does it reflect the willingness to adopt a fair amount of what was discussed here ;)

On defining the map canvas overlay widget as "ugly", what is used here is what most other map tools do (i.e. e.g. offset curve map tool, rotation map tool, scale map tool, etc.). If you feel like there's room for improvements for this well-established overlay, this can be discussed separately. We could give some tweaks a try (e.g. semi-transparent background?), but we're still a bit limited by what Qt widgets offers us.

Thanks for the reviews here, you've made this a better feature; keep them coming 🙏