Rework handling of panning to identified features - #4591
Conversation
We will use it to jump to the highlighted feature once we know the available area of the map that is not covered by the panel
- Use the renamed previewPanelHeight property instead of signal params
- Move all highlighting and form opening to a identifyFeature()
- Don't pan if whole feature is near visible map center - Center feature to visible map if it fits - Center clicked location to visible map if whole does not fit
- Add method to calculate the map extent required for a geometry to fit the visible part of the map canvas when covered by a drawer - Use animated zoom when identifying features from a list and not on map
|
might help resolve #3845 as well |
Withalion
left a comment
There was a problem hiding this comment.
Nice! It would be great to rebase on current master as well since some time passed.
| { | ||
| rendererPrivate.freeze('jumpTo') | ||
|
|
||
| let oldExtent = mapRenderer.mapSettings.visibleExtent |
There was a problem hiding this comment.
| let oldExtent = mapRenderer.mapSettings.visibleExtent | |
| const oldExtent = mapRenderer.mapSettings.visibleExtent |
| let tmpMinX = startMinX - percentage * 0.01 * ( startMinX - endMinX ) | ||
| let tmpMinY = startMinY - percentage * 0.01 * ( startMinY - endMinY ) | ||
| let tmpMaxX = startMaxX - percentage * 0.01 * ( startMaxX - endMaxX ) | ||
| let tmpMaxY = startMaxY - percentage * 0.01 * ( startMaxY - endMaxY ) |
There was a problem hiding this comment.
| let tmpMinX = startMinX - percentage * 0.01 * ( startMinX - endMinX ) | |
| let tmpMinY = startMinY - percentage * 0.01 * ( startMinY - endMinY ) | |
| let tmpMaxX = startMaxX - percentage * 0.01 * ( startMaxX - endMaxX ) | |
| let tmpMaxY = startMaxY - percentage * 0.01 * ( startMaxY - endMaxY ) | |
| const tmpMinX = startMinX - percentage * 0.01 * ( startMinX - endMinX ) | |
| const tmpMinY = startMinY - percentage * 0.01 * ( startMinY - endMinY ) | |
| const tmpMaxX = startMaxX - percentage * 0.01 * ( startMaxX - endMaxX ) | |
| const tmpMaxY = startMaxY - percentage * 0.01 * ( startMaxY - endMaxY ) |
| enabled: jumpExtentAnimator.enabled | ||
| } | ||
|
|
||
| onPercentageChanged: { |
There was a problem hiding this comment.
| onPercentageChanged: { | |
| onPercentageChanged: () => { |
either use lambda or anonymous function
|
|
||
| signal featureIdentified( var pair ) | ||
| // Holds the map coordinates of the point the user identified. NaN if identify was triggered from list of features | ||
| property point identifyLocation: Qt.point(NaN, NaN) |
There was a problem hiding this comment.
wouldn't it be better to just use null here instead, if the location is unknown?
| // Holds the map coordinates of the point the user identified. NaN if identify was triggered from list of features | ||
| property point identifyLocation: Qt.point(NaN, NaN) | ||
|
|
||
| signal featureIdentified( var pair, var point ) |
There was a problem hiding this comment.
| signal featureIdentified( var pair, var point ) | |
| signal featureIdentified( FeatureLayerPair pair, qgsPoint clickedPoint ) |
It's about time we expose FeatureLayerPair & QgsPoint to QML properly
|
|
||
| // point well inside the unobstructed area -> no pan needed | ||
| const QgsGeometry visiblePoint = QgsGeometry::fromPointXY( QgsPointXY( 20, 40 ) ); | ||
| pan = mUtils->whereToPanWhenIdentifying( visiblePoint, &ms, bottomOffset, QPointF( 20, 40 ) ); |
There was a problem hiding this comment.
| pan = mUtils->whereToPanWhenIdentifying( visiblePoint, &ms, bottomOffset, QPointF( 20, 40 ) ); | |
| pan = InputUtils::whereToPanWhenIdentifying( visiblePoint, &ms, bottomOffset, QPointF( 20, 40 ) ); |
| } | ||
| } | ||
|
|
||
| void TestUtilsFunctions::testWhereToPanWhenIdentifying() |
There was a problem hiding this comment.
Could we check here that the scale has not changed as well
| } | ||
| } | ||
|
|
||
| void TestUtilsFunctions::testDrawerCompensatedExtent() |
There was a problem hiding this comment.
As well here I would have case for:
- when the map is just panned
- when feature is bigger then current extent, so the map zooms out
- when feature is smaller then current extent
| // point geometry -> scale is kept, center shifts down by bottomOffset / 2 | ||
| // in screen space so the point is centered in the unobstructed part | ||
| const QgsGeometry point = QgsGeometry::fromPointXY( QgsPointXY( 20, 10 ) ); | ||
| QgsRectangle extent = mUtils->drawerCompensatedExtent( point, &ms, bottomOffset ); |
There was a problem hiding this comment.
| QgsRectangle extent = mUtils->drawerCompensatedExtent( point, &ms, bottomOffset ); | |
| QgsRectangle extent = InputUtils::drawerCompensatedExtent( point, &ms, bottomOffset ); |
| // non-empty bounding box -> zoom so the padded bbox fits the part of the | ||
| // canvas not covered by the drawer, centered in it | ||
| const QgsGeometry line = QgsGeometry::fromPolylineXY( { QgsPointXY( 10, 10 ), QgsPointXY( 30, 20 ) } ); | ||
| extent = mUtils->drawerCompensatedExtent( line, &ms, bottomOffset ); |
There was a problem hiding this comment.
| extent = mUtils->drawerCompensatedExtent( line, &ms, bottomOffset ); | |
| extent = InputUtils::drawerCompensatedExtent( line, &ms, bottomOffset ); |
Plan A
This PR is a rework on how we jump to geometries. It was initially just a way to mitigate a GEOS crash when identifying self-overlapping multipart geometries, but quickly evolved!
Plan B
If this is too much for a last minute change, we can avoid hitting the geos bug by slightly modifying the current approach: Perform bounding box intersections instead of geometry intersections.
This will change the existing behavior, eg identifying C shaped geometry that is partially visible will not recenter the map canvas to the center of the visible part of the geometry, but to the visible part of the geom bbox.
Testing notes: