Curved labels improved

[fritsvanveen]

Curved labels are now effectively rotated at the character mean line in stead of the base line. This means there is less overlap if the label is angled upwards and less extra space if the label is angled downwards.

[m-kuhn]

Nice!

Could you add a screenshot here that shows the difference?
Do you think it makes sense to make this an option or will it always produce better results?

[nyalldawson]

@m-kuhn see qgis/QGIS-Enhancement-Proposals#46

[nyalldawson]

@fritsvanveen thanks! I'll give this version a test and report back.

Don't worry about the failing unit tests - we'll need to update the curved label reference images to suit this, but that's a horrible job to do so I wouldn't expect you to tackle it

[fritsvanveen]

I have still a few concerns. I don't like the 0.9 in my code, but in my case it centers the label the best. It is not the optimum choice with other fonts. Also, for all upper case labels we don't have to consider descenders, so the label could be a little lower then for mixed case. It would be nice if the user could specify the offset.

[fritsvanveen]

@m-kuhn "will it always produce better results?" I think it will!

[nyalldawson]

Ok. I've just given this a solid test and despite my best efforts, I'm unable to find a single case where the new routine is worse than the old one.

The improvements are literally phenomenal!!

@fritsvanveen what do you think is needed before I can merge this?

I have still a few concerns. I don't like the 0.9 in my code, but in my case it centers the label the best. It is not the optimum choice with other fonts. Also, for all upper case labels we don't have to consider descenders, so the label could be a little lower then for mixed case. It would be nice if the user could specify the offset.

I'd try to avoid adding an option like this if we can... it's going to be very confusing for users exactly what it's there for. If the curved candidates generation code had knowledge of the label's ascent/descent, would that assist?

[fritsvanveen]

@nyalldawson Well, it is a rather minor point. The user can tweak the font size a bit to make descenders fit. I'd say, if you're happy, I'm happy. Merge it!

[mhugo]

@fritsvanveen Very nice improvement, thanks !
About this magic constant of 0.9, that's true it would be better if it could be deduced from the font / label. Could QFontMetrics help ? with boudingRect(), ascent() and descent(), we can deduce the ascent and descent of a given text I guess ...

[fritsvanveen]

@mhugo As I told Nyall already it really is a minor issue. Perhaps something for the future.
However, I think the offset should be constant during the rendering, based on the font (and use of it), not on each individual label.

[fritsvanveen]

@nyalldawson I noticed you reverted the code. Is it a unit test problem?

[nyalldawson]

@fritsvanveen yes - I need to rework how the test images are generated, so not related to this PR. I'm working on it :)

[fritsvanveen]

@nyalldawson Is this fix planned for the next PR of 2.16? Is it true that there will no 2.18, but work will commence for 3.0? Still, found another bug https://hub.qgis.org/issues/15341

[nyalldawson]

@fritsvanveen I'll backport it to 2.14/2.16/2.x too. Working on it now

[fritsvanveen]

@nyalldawson I fixed another labelling problem
Not sure how to generate a proper pull request for just this change.
Forget the commit for the remainder function. This was just to get the build system working on my computer. Sorry.

[nyalldawson]

@fritsvanveen I've pushed all these changes to master and backported to 2.14/2.16. I just need to backport your last fix to the master_2 branch and I'll close this PR.

Really nice stuff - i'm impressed you've managed to navigate the badly-documented pal classes!

Pro tip: if you state "fixes #15341" in your git commit, the corresponding ticket on hub.qgis.org will be auto-closed. It's also a good idea to make sure the first line in the commit message is descriptive enough to stand alone, ie "Fix perimeter labeling when geometry is clipped" is better than "fix #15341", as it makes browsing the git history/using git blame easier.

[nyalldawson]

@NathanW2 can we add a "needs backporting" label?

[NathanW2]

@nyalldawson I don't have admin rights. @timlinux does though.

[NathanW2]

Thanks @jef-n

[nyalldawson]

Ok, merged back to master_2. all done now