use single snapper

[3nids]

as discussed in #5, I tried with a single snapper and this improves the speed a lot. This is not lightning fast, but this is clearly usable on my big project (unusable before).

Cheers, Denis

[3nids]

Hi, Not 100% sure but in c++, the results are passed as reference so I believe once the snapping is finished the results get out of scope. That's why you need to make a copy.

Regarding snapping, you can still call it once, just use SnapWithResultsWithinTolerances instead of SnapWithOneResult and you go through all the results first for vertices, and return the first result for segment (see the last commit in pull request). They are in the correct order since current layer was put on top (and then it's by drawing order).

Also, there was a typo for snapLayer.mSnapTo = QgsSnapper.SnapToVertexAndSegment (no spaces after the dot).

Should be better! Cheers, Denis

[olivierdalang]

Hi ! I merged this, but now upon testing it seems it introduces problems.

In the PR, you commented out self.disableBackgroundSnapping() in createSnappingPoint.

This means that when a snapping point is created in the memory layer, the other normal layers's snaping is no more disabled. When the memory-layer snapping point is too near from an existing normal snapping point, it is possible that the snap is made to that existing point.

To reproduce : create some geometries and enable snapping on their layer. Enable CadInput, lock coordinates to a point near those geometries (but not on a snapping point), zoom out to have all geometries be 1px wide, and click. Sometimes the point will be created as expected on the locked coordinates, but sometimes it will be created on a regular snapping point.

So I'm afraid we have to keep that self.disableBackgroundSnapping() call, which may be the cause for that performances issue even more than the _toMapSnap() method (for which your implementation is much cleaner, thanks !)... I'll push a commit to master with that restored right now, can you try it with your large dataset ?

[3nids]

You're right! I will test it, I suppose it would even faster since there is a snap less to perform.

Anyway, I am not a huge fan a this idea of changing the settings of the project. I would ask to the list if there's a better way.

I see that the snapper is a private property of QgsMapToolEdit. So I would suggest to add a method enable/disableSnapping in the map tool class (there would also be an enable/disable method in QgsMapCanvasSnapper). You could then use the mapToolChanged signal from the mapCanvas to get the new tool and disable its snapping. To be safe, we could define that snapping is reenabled every time a map tool is activated.

What do you think?

[olivierdalang]

@3nids I'm not sure I understand what you mean.

To make this clear (its hard not to get lost) : there's two moments where the plugins makes use of snapping.

The "normal" case on line 109 (self.snapPoint, self.snapSegment) = self._toMapSnap( event.pos() ), where we simply want to get the currently snapped point and segment. This should work with no hack, since we want normal snapping to occur, but we still use a small hack since the native snapping priority is flawed (see issue http://hub.qgis.org/issues/7058). That's the part your PR improved since before that the plugin was uselessly changing snapping settings here too.

The "input" case on lines 144-147 self.createSnappingPoint() modifiedEvent = QMouseEvent( event.type(), self._toPixels(self.p3), event.button(), event.buttons(), event.modifiers() ) QCoreApplication.sendEvent(obj,modifiedEvent) self.removeSnappingPoint() where we have to rely on native QgsMapCanvas's snapping to be able to input a precise point while sending only an imprecise pixel-level mouse event. For now, we have no control on how QgsMapCanvas's makes its snapping except by changing the snap settings. Even if we have access to that QgsMapCanvas's snapper (or QgsMapToolEdit's snapper, which I believe/hope will be the same instance), we need to disable/reenable snapping on other layers just before/after sending the mousePress and mouseRelease events. Doing this directly on the QgsMapCanvasSnapper/QgsMapToolEdit.mSnapper.setSnapLayers would for sure be cleaner, but we have no access to it with current API.

Still, the real clean way to do this is to allow inputting mapCoordinates in QgsTool...