Bug report #11814
PAL line label placement does not consider map rotation
|Affected QGIS version:||master||Regression?:||No|
|Operating System:||Easy fix?:||No|
|Pull Request or Patch supplied:||No||Resolution:||fixed/implemented|
|Crashes QGIS or corrupts data:||No||Copied to github as #:||20037|
Labels placement for lines does not properly consider map rotation.
With "horizontal" placement all labels are horizontal, which is nice, but placement is somewhat offsetted from the expected one.
With "parallel" placement the labels are parallel to the pre-rotated lines, so stop being parallel once lines are rotated.
I'm happy to look at it but would need help by Larry
#5 Updated by Sandro Santilli about 5 years ago
QgsPalLayerSettings::calculateLabelSize is the earlier point of failure I've found for today.
It does indeed looks like the function assumes QgsMapToPixel.toMapCoordinates( width, height ) would give correct width and height in presence of rotation, which is obviously not the case.
Maybe we could add another function to QgsMapToPixel to transform sizes... (rotation would not affect them, nor translation but only scale, isn't that correct?)
#6 Updated by Sandro Santilli about 5 years ago
@dakarto: am I right that candidate label boxes are NOT stored as rotated, when being parallel to lines ?
// save the rect
rect.moveTo( outPt.x(), outPt.y() );
mCandidates.append( QgsLabelCandidate( rect, lp->getCost() * 1000 ) );
in src/core/qgspallabeling.cpp, where label alpha (angle) is not taken in consideration
#7 Updated by Sandro Santilli about 5 years ago
Some support for map rotation in labels rendering is been pushed here:
Larry your review is welcome. I had the impression a refactoring of the labeling system would be useful (like to work in pixel space rather than in geographical units space, for the sake of collision prevention, and to pass the whole map QTransform to correctly convert from one space to the other) -- the commits in the PR do not attempt to do any refactoring.
#8 Updated by Larry Shaffer about 5 years ago
Correct me if I am wrong (have not had time to review your map rotation committed changes), but unless the features are pre-rotated and (optionally) clipped prior to going into the labeling engine (right before PAL registration), it will be a major refactoring to support map rotation. Are the features not being pre-rotated? Note: the engine works with temporary duplicates of the geometries.
I think refactoring the engine output (and calculations) to be relative to pixel space might not gain anything here, plus PAL library works with map units, and the map scale is passed in. So, it would also require a refactoring of the PAL library as well. I think working only in map units is the correct approach to the labeling engine, then convert to output device units/resolution during output rendering.
However, I have not before considered doing everything at output resolution pixel dimensions as a means of simplifying the engine. You may want to ask Martin Dobias what he thinks about such an approach.
#9 Updated by Sandro Santilli about 5 years ago
The vectors are not pre-rotated by the provider, but the MapSetting's matrix (QgsMapToPixel) would be able to both rotate and scale to the output coordinate space.
I noticed "scale" is accepted by the label engine. In order to rotation to be taken in consideration it should also take rotation and rotation center (output size).
All in all having PAL directly work on the output coordinate space should simplify things as it would not even need to be passed a scale.
Why do you think working in map units is the correct approach to the labeling engine ? After all labels are configured in output device terms (size, collision).
@wonder-sk what do you think about pre-scaling as well, rather than passing scale in ?
Where should I look at to pre-rotate them for the PAL registration ?
#10 Updated by Sandro Santilli about 5 years ago
- Assignee changed from Larry Shaffer to Sandro Santilli
- Resolution set to fixed/implemented
- Status changed from Open to Closed
Label placement (for horizontal and parallel) is supported as of commit 917cee0510cdba4a52cae4a785a974808ecb7a56
I've filed #11856 for the refactoring task
#14 Updated by Sandro Santilli about 5 years ago
Another problem with the pre-rotation approach is that the current PAL code is full of places where the current QgsMapToPixel is used to transform map points to device points (for label placement). All such pieces of code would need to use a modified QgsMapToPixel so that the rotation portion of it is removed. To make things worst some code snippets use the QgsMapToPixel registered in the QgsMapSettings (mMapSettings) and others in the QgsRenderingContext (context).
I'm not sure anymore if pre-rotating is any better than building on the path already taking of further considering the rotation component of the already-used QgsMapToPixel... One contribution in that direction arrived recently: https://github.com/qgis/QGIS/pull/1750