Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Read SLD TextSymbolizer set units to pixels #33725

Merged
merged 8 commits into from
Jan 16, 2020

Conversation

rldhont
Copy link
Contributor

@rldhont rldhont commented Jan 10, 2020

Description

The SLD unit is pixels so font size unit, buffer unit, offset unit and distance unit have to be set to render pixels unit.

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include Fixes #11111 at the bottom of the commit message
  • I have read the QGIS Coding Standards and this PR complies with them
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
  • I have evaluated whether it is appropriate for this PR to be backported, backport requests are left as label or comment

@rldhont rldhont added Labeling Related to QGIS map labeling Bug Either a bug report, or a bug fix. Let's hope for the latter! backport release-3_10 labels Jan 10, 2020
@rldhont rldhont added this to the 3.12.0 milestone Jan 10, 2020
@luipir
Copy link
Contributor

luipir commented Jan 10, 2020

LGTM... BTW @rldhont did you check if during SLD output of the font, it is always converted to pixel units? (could be a possible bug)

@luipir
Copy link
Contributor

luipir commented Jan 10, 2020

@rldhont waiting for test fix to accept the PR

@rldhont
Copy link
Contributor Author

rldhont commented Jan 10, 2020

@luipir I have check that every units are converted to pixels.

@rldhont
Copy link
Contributor Author

rldhont commented Jan 10, 2020

@luipir code fixed

@nyalldawson
Copy link
Collaborator

One consideration here is that pixel unit sizes have very limited use in QGIS. If you set the font size and parameters to a pixel sized unit, then any outputs made from that map in hi resolutions (such as from a print layout) will have tiny sizes. So it depends what the intention is here... if it's for exact 1:1 match of the SLD then the change is good, but if it's to aid users with making usable maps from an SLD style, then you're much better doing a conversion to a mm based size!

@rldhont
Copy link
Contributor Author

rldhont commented Jan 11, 2020

The intention is to do the same as createSymbolizerFromSld functions, so it's for exact 1:1 match of SLD.

Some enhancements have to be done in the QGIS SLD readers:

  • reorganize the API: functions are in QgsVectorLayer, QgsSymbolLayerUtils and QgsOgcUtils
  • translate from pixel unit to mm ot pt

@luipir
Copy link
Contributor

luipir commented Jan 13, 2020

@nyalldawson do you agree to merge postponing enhancement listed by @rldhont ?

@luipir
Copy link
Contributor

luipir commented Jan 13, 2020

@aaime FYI in case affect your SLD based workflows (I suppose it does not)

@nyalldawson
Copy link
Collaborator

I don't have a strong opinion either way about this - I just wanted to point out the inherent issue with pixel based units

@aaime
Copy link
Contributor

aaime commented Jan 13, 2020

SLD indeed works in pixels by default. There are no other screen units, if one wants, it is possible to use ground units, meters or feet, but that has an explicit attribute at the symbolizer level (check the Symbology Encoding extension, look for the "uom" attribute).
Looks good to me.

Ah, about print-outs, both MapServer and GeoServer allow to specify a "DPI" vendor parameter in that case, which ends up rescaling the pixel units and, more importantly, the scale denominators calculation, according to the output DPI. SLD assumes roughly 91dpi by default. It would seem like a useful behavior for QGIS as well (I understand it's out of this PR scope).

@luipir
Copy link
Contributor

luipir commented Jan 13, 2020

but that has an explicit attribute at the symbolizer level

@rldhont so we can't assume pixel as default without checking the context!
e..g if no Unit is specified at symbolizer level => assume pixel as default

@rldhont
Copy link
Contributor Author

rldhont commented Jan 13, 2020

@aaime QgsSymbolLayerUtils::decodeSldUom is not used and tested.

QgsUnitTypes::RenderUnit QgsSymbolLayerUtils::decodeSldUom( const QString &str, double *scaleFactor )
{
  if ( str == QLatin1String( "http://www.opengeospatial.org/se/units/metre" ) )
  {
    if ( scaleFactor )
      *scaleFactor = 1000.0;  // from meters to millimeters
    return QgsUnitTypes::RenderMapUnits;
  }
  else if ( str == QLatin1String( "http://www.opengeospatial.org/se/units/foot" ) )
  {
    if ( scaleFactor )
      *scaleFactor = 304.8; // from feet to meters
    return QgsUnitTypes::RenderMapUnits;
  }

  // pixel is the SLD default uom. The "standardized rendering pixel
  // size" is defined to be 0.28mm x 0.28mm (millimeters).
  if ( scaleFactor )
    *scaleFactor = 1 / 0.00028; // from pixels to millimeters
  return QgsUnitTypes::RenderMillimeters;
}

What are the available uom values ?

@aaime
Copy link
Contributor

aaime commented Jan 13, 2020

Quoting from the Symbology Encoding 1.1 specification:

The following uom definitions are recommended to be used:
<PolygonSymbolizer uom="http://www.opengeospatial.org/se/units/metre"/>
<PolygonSymbolizer uom="http://www.opengeospatial.org/se/units/foot"/>
<PolygonSymbolizer uom="http://www.opengeospatial.org/se/units/pixel"/>

Pixel is interpreted as a paper unit, referring to the size of the map, while metre, foot and
other similar units are “ground” units referring to the actual size of real-world objects. All
values set inside this Symbolizer shall use this unit for drawing the corresponding
elements. It is also possible to use pixel values inside a Symbolizer that uses a uom: px
has to be appended to the corresponding values in this case (e.g. 5px stands for 5 pixel).

@rldhont
Copy link
Contributor Author

rldhont commented Jan 13, 2020

@aaime can you review the code ?

@luipir
Copy link
Contributor

luipir commented Jan 13, 2020

@rldhont test seems does not cover in case of "uom" but yust default (pixels) https://github.com/qgis/QGIS/pull/33725/files#diff-106be592650b55ccae79c83eecd15cebR4477
Am I right?

@rldhont rldhont force-pushed the fix-read-sld-textsymbolizer-units branch 2 times, most recently from d39dbc3 to 3a66cd8 Compare January 15, 2020 18:16
@rldhont rldhont force-pushed the fix-read-sld-textsymbolizer-units branch from 3a66cd8 to 95c3b02 Compare January 16, 2020 07:44
@rldhont rldhont force-pushed the fix-read-sld-textsymbolizer-units branch from 95c3b02 to dbb53be Compare January 16, 2020 10:04
@rldhont rldhont merged commit ab9cbe0 into qgis:master Jan 16, 2020
@rldhont rldhont deleted the fix-read-sld-textsymbolizer-units branch January 17, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Labeling Related to QGIS map labeling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants