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

[Feature] Point at circle intersection #45904

Closed
wants to merge 50 commits into from

Conversation

Koyaani
Copy link
Contributor

@Koyaani Koyaani commented Nov 5, 2021

Description

This PR adds a point construction tool. The user can create a point at one of the 2 intersections of 2 circles that he has defined.

This functionality can already be done with the advanced digitizing tools and temporary layers. This tool simplifies this process.

demo

This tool is inspired by external solutions. Moreover, a prototype with Python has been made to validate its pertinence.

A new bar "cogo" has been created, in the future other construction tools of this type will be added (eg. point at line intersection).

Code

I made the QgsSnapPoint class by overwriting only cadCanvasPressEvent. All the rest of the logic has been put into the dialog class.
I moved the static method digitizingStrokeColor to public instead of protected so that it could be accessed outside of a QgsMapToolEdit class.

As for the activation / deactivation of the icon, I did something that is not usually done. I disabled it by default and then enabled it in the one relevant case (an editable point layer). Otherwise, I was afraid to forget a case where it should be disabled.

With the advice of @lbartoletti

Sponsored by: Métropole Européenne de Lille @Jean-Roc

@github-actions github-actions bot added this to the 3.24.0 milestone Nov 5, 2021
@nyalldawson
Copy link
Collaborator

Thanks for the contribution @Koyaani -- looks like a.handy functionality to have available!

Please don't merge this before it's had a UX review by myself or @nirvn or @DelazJ -- the new dialog and approach used here don't fit well with other qgis map tools and a rethink in how we want to expose tools like this is needed first.

@uclaros
Copy link
Contributor

uclaros commented Nov 7, 2021

Imho, UX wise, such functionality should be somehow integrated to the advanced digitizing panel, so that all digitizing tools can benefit from it and not only point creation.

@lbartoletti
Copy link
Member

@nyalldawson

the new dialog and approach used here don't fit well with other qgis map tools and a rethink in how we want to expose tools like this is needed first.

@uclaros

Imho, UX wise, such functionality should be somehow integrated to the advanced digitizing panel

This was my first idea months/years ago. But, most of the users I meet during training, advice, assistance prefer to use simple and direct features rather than using this panel. This was also confirmed to us by @Jean-Roc, whose users maintain certain habits and prefer dedicated tools. That's why we propose this interface, which is directly inspired by current tools (CAD and GIS) on these features.

We are obviously listening to you to find the most integrated solution to QGIS while remaining attentive to the needs of users for whom these tools (others will follow) are intended.

@lbartoletti lbartoletti added Changelog Items that are queued to appear in the visual changelog - remove after harvesting Digitizing Related to feature digitizing map tools or functionality Feature Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo. NOT FOR MERGE Don't merge! labels Nov 8, 2021
@github-actions
Copy link

github-actions bot commented Nov 8, 2021

@Koyaani
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!

@nyalldawson
Copy link
Collaborator

@lbartoletti

Well, I guess it depends on the primary use case of these tools.

  • If they are for creation of point features only, then I'd suggest a processing algorithm might be a better approach here. It's less impactful on the QGIS gui and more flexible.
  • If they are for digitizing features, then I agree with @uclaros and this functionality belongs in the advanced digitizing dock. That's where all the other similar construction tools live right now,and it's the expected place to find this new functionality. (It also ensures it will "just work" with ALL the tools which hook into the CAD dock, without any extra work). If you don't feel that the CAD dock is good UX, then that needs to be addressed as a complete piece of work, but not by just throwing another toolbar onto the QGIS interface and splitting these tools up.
  • If it's meeting a specific requirement of a select group of users who HAVE to have it exposed like its done in the PR, then I'd suggest a plugin is more appropriate.

Basically, we can't just keep throwing more buttons and toolbars and dialogs onto the QGIS interface and expect things to scale well for users. Honestly, we should have hit this point years ago and discouraged the explosion of actions back then, but we didn't, and I'm becoming more and more hard-line about getting submitters to rethink any additional actions they add (just like this!).

@Jean-Roc
Copy link
Member

Jean-Roc commented Nov 9, 2021

Hi Nyall, while I agree with the need to curb the UI's growth (3.22 gained two new default toolbars) and an avid user of the existing digitizing dock, the latter is not fit for most of our digitizing users accustomed to have an action = an action (most of them coming from CAD and GIS tools).

We can find similar use cases in the advanced digitizing or shape panels where you can find a number of tools where each is available for a dedicated task, some of them have a similar behavior by opening a window to get a custom value input. Would adding this specific tool to one of these panels be an acceptable proposal ?

@nyalldawson
Copy link
Collaborator

Would adding this specific tool to one of these panels be an acceptable proposal ?

Not for me, no. I don't think moving the action to a different toolbar solves the underlying UX issue at all.

@Koyaani
Copy link
Contributor Author

Koyaani commented Nov 15, 2021

@nyalldawson @uclaros We are not against moving the action (and the future ones) to the advanced digitizing dock. Indeed, the logic would be to put these actions in this panel. But how do you see the integration of these actions in this panel?

The feedback we get of QGIS users who do CAD is that they prefer to have for each action, a button in a toolbar. This corroborates with the other existing softwares. We insist a lot because we think that changing this logic will lose the current and future users. Do we want to change that? Couldn't a CAD user build his own QGIS profile?

@nirvn, @DelazJ @SrNetoChan Do you have an opinion or other elements on this question? And in any case, if the conversation stays here, we'll change this aspect so that the PR is merged.

@SrNetoChan
Copy link
Member

I find this tool very specific (or maybe it's not clear what is its use case) and for that reason, I think it would be better in a plugin. If it could be integrated into another tool, like the advanced digitizing panel, that would be better.

I will not doubt your experience with (?ex-CAD?) users, and they need/desire for single action single button, but QGIS is used by a huge variety of users, not all coming from CAD, and for those, trying to keep the UI simple and intuitive is quite important. For entry-level users in training, the number of actions in the toolbars and menus is already quite challenging, so it would be better if we could find another solution.

@SrNetoChan
Copy link
Member

You probably already have thought about all those ideas, and this might be just too much work to implement, or make no sense, but if the advanced digitizing panel could freeze a certain condition and then accumulate with the next, one could:

  • Click the center of the first circle and fix the distance of the circle
  • Save that condition
  • Click the center of the next circle and set the distance,
  • Exit building mode and it would only be possible to insert the points in the 2 intersections of the circles.

Another totally different approach would be a new mode in the advanced digitizing panel that would allow creating reference geometries to use for digitizing (skipping the need to create them in memory layers) In that case one could use the shape tools to create circles. More buttons anyway though...

@nyalldawson
Copy link
Collaborator

@Koyaani

But how do you see the integration of these actions in this panel?

What about something like this:

  1. There's a new button for "COGO" tools in the toolbar of the advanced digitizing dock
  2. Clicking this switches the advanced digitizing dock to a "toolbox" style panel, which has a button for each of these tools. Kind of like this type of toolbox:
    image
  3. The value entry widgets should then live in a panel which shows in the Advanced Digitizing dock widget too. (You could switch this dock to use the QgsPanelWidgetStack class to get the nice nested inline panel approach)

@uclaros
Copy link
Contributor

uclaros commented Nov 18, 2021

Half of this feature's functionality already exists on the advanced digitizing toolbar, it's just called Distance instead of Circle. So what is actually missing is a way to add a second, independent reference point for a second Distance (circle 2).
My take for this would be to somehow hold all locked restrictions and offer the user a new, empty panel and allow to define a new set of locked restrictions - maybe in a new tab or with a back button to return to the first set of restrictions.
So the user sets a distance restriction for his first circle, then presses the Add second set of restrictions button and is presented with a new empty advanced digitizing form to define his second distance restriction. Both sets of restrictions are active at the same time so the user picks the circle intersection that suits him.

This method will also allow for two bearings intersections to be achieved, or combination of bearing and distance.

Some care should be taken for cases when the two sets of restrictions do not have a valid solution, ie the first tab's restrictions do not intersect with the second tab's restrictions (the two circles do not intersect in the PR's use case)

@Koyaani
Copy link
Contributor Author

Koyaani commented Nov 26, 2021

So, I will implement the @nyalldawson's proposition. This is probably the best compromise between keeping the simplicity of 1 action = 1 button and limiting the addition of new bars.

@uclaros
Copy link
Contributor

uclaros commented Nov 26, 2021

Maybe it would be best to open a QEP first, describing your use case and your plans for other similar tools you mentioned.

@nyalldawson
Copy link
Collaborator

Maybe it would be best to open a QEP first, describing your use case and your plans for other similar tools you mentioned.

+1

@nyalldawson
Copy link
Collaborator

I'll close this for now -- please reopen when it's ready for re-review

@nyalldawson nyalldawson closed this Dec 1, 2021
@zacharlie zacharlie added ChangelogExclude Items excluded from changelog harvesting and removed Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels May 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ChangelogExclude Items excluded from changelog harvesting Digitizing Related to feature digitizing map tools or functionality Feature Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo. NOT FOR MERGE Don't merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants