Bug report #4454

multiline labels overlap in the labeling-ng engine

Added by Mathieu Pellerin - nIRV almost 9 years ago. Updated almost 9 years ago.

Status:Closed
Priority:Normal
Assignee:Martin Dobias
Category:Map Canvas
Affected QGIS version: Regression?:No
Operating System: Easy fix?:No
Pull Request or Patch supplied:Yes Resolution:fixed
Crashes QGIS or corrupts data: Copied to github as #:14384

Description

The recently added expression-based label feature exposed an issue with multiline support in the labeling-ng engine.

The labeling-ng placement mechanism appears to only consider the width of a label's last line, resulting in major overlaps if the first line's length is greater than the last one. (see screenshot A map-preylang-labeloverlap.jpg)

E.G.
Line #1: Big Plantation Company
Line #2: 500 Ha

I think the above assessment must be pointing in the right direction. If I change the multiline label to have the longest string on the last line, the labeling engine doesn't render overlapping labels. (see screenshot B map-preylang-labelclean.jpg)

This issue will have a greater impact on users when the next qgis version will be released with the expression-based labeling, hope this issue' description is helpful.

map-preylang-labeloverlap.jpg - screenshot A (672 KB) Mathieu Pellerin - nIRV, 2011-10-27 08:24 PM

map-preylang-labelclean.jpg - screenshot B (667 KB) Mathieu Pellerin - nIRV, 2011-10-27 08:24 PM

History

#1 Updated by Mathieu Pellerin - nIRV almost 9 years ago

I'm pretty sure I found the problem.

The multiline label's width is determined in the void QgsPalLayerSettings::calculateLabelSize() function. The relevant code is this:

QRectF labelRect = fm->boundingRect( text );
double w, h;
if ( !multiLineLabels )
{
  w = labelRect.width() / rasterCompressFactor;
  h = labelRect.height() / rasterCompressFactor;
}
else
{
  QStringList multiLineSplit = text.split( "\
" );
  h = fm->height() * multiLineSplit.size() / rasterCompressFactor;
  w = 0;
  for ( int i = 0; i < multiLineSplit.size(); ++i )
  {
    double width = fm->width( multiLineSplit.at( i ) );
    if ( width > w )
    {
      w = width;
    }
    w /= rasterCompressFactor;
  }
}

The problem is in the for loop that compares each line's width to find out the longest one. The rasterCompressFactor division applied to the width should occur after the for loop has found the longest width, no within the loop. The current code break the if (width > w) condition as it's not comparing two width but rather a width and a width / rasterCompressFactor.

So, the solution is to move w /= rasterCompressFactor; after the loop. Yay, a one line change fix! :)

#2 Updated by Mathieu Pellerin - nIRV almost 9 years ago

The code should be:

QRectF labelRect = fm->boundingRect( text );
double w, h;
if ( !multiLineLabels )
{
  w = labelRect.width() / rasterCompressFactor;
  h = labelRect.height() / rasterCompressFactor;
}
else
{
  QStringList multiLineSplit = text.split( "\
" );
  h = fm->height() * multiLineSplit.size() / rasterCompressFactor;
  w = 0;
  for ( int i = 0; i < multiLineSplit.size(); ++i )
  {
    double width = fm->width( multiLineSplit.at( i ) );
    if ( width > w )
    {
      w = width;
    }
  }
  w /= rasterCompressFactor;
}

I don't have the environment set up to come up with a proper patch, hope someone can do it on my behalf.

#3 Updated by Nathan Woodrow almost 9 years ago

  • Pull Request or Patch supplied changed from No to Yes
  • File 0001-Fix-multiline-overlap.patch added

Patch for fix.

#4 Updated by Mathieu Pellerin - nIRV almost 9 years ago

Hey Nathan, thanks, that was quick :)

It might be good to add this patch to your enable multiline per default pull request (https://github.com/qgis/Quantum-GIS/pull/53).

Cheers and thanks again.

#5 Updated by Nathan Woodrow almost 9 years ago

  • File deleted (0001-Fix-multiline-overlap.patch)

#6 Updated by Nathan Woodrow almost 9 years ago

  • Assignee set to Martin Dobias

Agreed. Now part of pull request.

#7 Updated by Martin Dobias almost 9 years ago

  • Resolution set to fixed
  • Status changed from Open to Closed

Pull request merged!

Also available in: Atom PDF