Add feature tool: add topological point to each editable layer instead of only the snapped layer #57723
Conversation
|
Thanks for looking into this long lasting issue! IMHO, adding snapped points as topological points to all editable layers is not enough. We should be adding the whole geometry as topo points to all line/poly editable layers of that canvas ( we currently do so only for the active layer ), as there might be vertices added with the Advanced Digitizing Tools. Also, those points should be added to the individual layer's edit buffer so it is an undoable step, similar to how What do you think? |
|
I think that does make sense, and it should make the topological editing more consistent. Unless I misunderstood your point, the points are currently already added to the edit buffer of the editable layers, you can undo the changes to each layer the points were added to. |
They are indeed, but there is no description for the undo step. If it gets wrapped within a |
|
Oh yes, I understand now. I think that's a good addition as well. Thank you for the suggestions. I've made the changes suggested by @uclaros. |
Can you clarify? Are you saying the points get added to all layers which are in edit mode, or ALL layers which CAN be turned into editable layers? |
Oh, no, that would be quite terrifying! The reasoning is that other geometry's vertices may be affecting topology even if thay are not snapped, eg. added to exact coords using cad tools. This proposed change was added with the subsequent commits |
Another case is when using stream/tracing digitizing, adding by snapped points would not (in its current form) add the topological points.
This did cross my mind as well as obviously adding the topological points for the whole geometry is going to be slower than adding just the snapped points. I did do some manual testing and my impression overall is that it's not as slow as I feared, but once you're adding several hundred new topological points you do start noticing a bit of a spike. Of course with more layers it's going to be worse. However I would say that comparing it to other digitizing tools, f.e. move feature or adding a feature with avoid overlap and topological editing it's about on par.
edit. nevermind, that's not actually going to work how I thought it would. |
- Don't differentiate current layer - Get layers from canvas, not project - Init result variable at failure state
|
If we choose to take into account ALL currently edited layers (which would be a nice things to do), we also need to modify other maptool (qgsvertextool for instance, but also reshape/rotate..) in order to stay consistent on the whole application. I'm also concerned about performance implication depending on the number of edited layer. Maybe it could be an option: add topo points on all edited layer OR on only modified and snapped points |
|
Well for my part I'm okay with adding it as an option, but just to clarify, in this case if the user chooses to add only modified and snapped points would those still be added to each editable layer? Because if not, the original issue in #56689 would persist unless the other option was chosen. Also where would one change the option? I'd probably add it to the snapping toolbar as a drop-down option similar to the overlap options. |
|
I really cannot come up with a use case for the current behavior, ie. only add points to snapped layer. Even if there is one, it would still probably be simpler to stop editing the layers that one does not want to receive the topo points than have a specific setting to change behavior! I agree that other tools should follow this behavior but I guess it's outside the scope of this PR. Maybe we could eventually move the logic into |
Agreed, forget about the settings @JuhoErvasti
I'm not sure that's doable to add the logic into QgsMapToolEdit, all map tools are a bit specific |
|
Actually, this now results in two undo steps for the current layer - one for added feature, one for topo points - and this will lead to coonfusion. So maybe the active layer should still be treated differently after all. |
|
Hmm. That is the way it worked before and if I just change this so the current layer is handled differently it'll still create a different undo step for the topo points (with an empty description too). This is because the topo points are added after I agree it might be confusing though, but IMO fixing it is a bit outside of the scope of this PR and warrants a separate issue. You'd have to change the logic fairly substantially to get both the 'add feature' step and 'add topo points' actions to the same undo step. |
|
In that case I'm ok to merge! |
Description
Fixes #56689 (which is a duplicate of #37824).
Instead of adding the topological point to the snapped layer in the QgsPointLocator::Match a topological point is added to each editable layer. This is done by iterating over editable layers in QgsMapToolAddFeature. One another way might've been to add a list of layers which have a segment that match the snap point to Match but that'd be a more substantial change to core so I've opted to handle all of it in the map tool itself.
Funded by the National Land Survey of Finland.