Bug report #11814

PAL line label placement does not consider map rotation

Added by Sandro Santilli almost 5 years ago. Updated over 4 years ago.

Status:Closed
Priority:Normal
Assignee:Sandro Santilli
Category:Labelling
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

Description

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

History

#1 Updated by Sandro Santilli almost 5 years ago

  • Tag set to rotation

#2 Updated by Sandro Santilli almost 5 years ago

Am I reading the code correctly that the PAL library does all its work in geographical units rather than device units ?
That is, it performs no coordinate transformation to device until it's time to rendering them (ie: not during problem resolution?)

#3 Updated by Sandro Santilli almost 5 years ago

Debugging prints confirm, pal::LabelPosition getX(), getY(), getWidth() and getHeight() all return map (geographical) units, not device (pixels).

#4 Updated by Sandro Santilli almost 5 years ago

And, when rotation is active, the height and the width are wrong.

#5 Updated by Sandro Santilli almost 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 almost 5 years ago

@dakarto: am I right that candidate label boxes are NOT stored as rotated, when being parallel to lines ?
as per:

// 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 almost 5 years ago

Some support for map rotation in labels rendering is been pushed here:
https://github.com/qgis/QGIS/pull/1729

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 almost 5 years ago

Hi Sandro,

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 almost 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 almost 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

#11 Updated by Sandro Santilli almost 5 years ago

  • Status changed from Closed to Reopened
  • Resolution deleted (fixed/implemented)

Reopening to try the pre-rotation approach.

#12 Updated by Sandro Santilli over 4 years ago

It looks like there's no function to rotate a QgsGeometry right now, so this won't be as simple as I was hoping for.
Or am I missing a code path to rotation ? The renderer must be doing it, right ?
http://qgis.org/api/classQgsGeometry.html

#13 Updated by Sandro Santilli over 4 years ago

GEOS does not have a rotation method either. The enhancement was requested over 10 months ago:
http://trac.osgeo.org/geos/ticket/687

#14 Updated by Sandro Santilli over 4 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

#15 Updated by Sandro Santilli over 4 years ago

Experimental pre-rotation code is here: https://github.com/qgis/QGIS/pull/1757
Note how working on map (rather than device) coordinate space makes things complex (ref #11856)
Comments welcome

#16 Updated by Sandro Santilli over 4 years ago

  • % Done changed from 0 to 100
  • Status changed from Reopened to Closed
  • Resolution set to fixed/implemented

Pre-rotated approach pushed to master with commit 9c6fe0614975049c55035325f80590f830c3d856 -- please file new tickets if other issues arise

Also available in: Atom PDF