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

Rotation labels #1729

Closed
wants to merge 6 commits into from
Closed

Rotation labels #1729

wants to merge 6 commits into from

Conversation

strk
Copy link
Contributor

@strk strk commented Dec 11, 2014

Add some map rotation knowledge in label renderer.
Horizontal and parallel line labels are now drawn correctly.
Curved labels are still suffering and I hadn't tested backshadow and such.

\cc @dakcarto for review

Related hub ticket: http://hub.qgis.org/issues/11814

Sandro Santilli added 2 commits December 10, 2014 11:00
See http://hub.qgis.org/issues/11814
NOTE: the map rotation should likely be considered at LabelPlacement
      configuration rather than at rendering time
@strk
Copy link
Contributor Author

strk commented Dec 11, 2014

One example issue with putting the map support in the renderer is (for example) that upside-down labels are not flipped when the upside-down occurs due to map rotation...

@strk
Copy link
Contributor Author

strk commented Dec 11, 2014

Subtle differences in tests again: http://dash.orfeo-toolbox.org/testDetails.php?test=27343142&build=166204

And if we update images, I'm afraid, they'll need to be updated more later, when changing the rendering pipeline again. Could we just raise tolerances instead @dakcarto ?

@strk
Copy link
Contributor Author

strk commented Dec 11, 2014

Still fails even with the short-cut. Reminds me of the first rotation support commit (read: it's crazy).

Sandro Santilli added 3 commits December 11, 2014 13:55
It's just a test to try at understanding the unstable test results
from TravisCI (extremely frustrating). Again, this commit will need
to be REVERTED before merging to master.
.. this is getting ilarious
painter->translate( QPointF( outPt.x(), outPt.y() ) );
painter->rotate( -lp->getAlpha() * 180 / M_PI );
QRectF rect( 0, 0, outPt2.x() - outPt.x(), outPt2.y() - outPt.y() );
rect = QRectF( 0, 0, outPt2.x() - outPt.x(), outPt2.y() - outPt.y() );
painter->drawRect( rect );
painter->restore();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this rect assignment methodology change necessary? Just masks git blame results for no apparent gain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the last 3 commits are just expeirments for travis, if you really want to see the real changes drop the last 3 (or even 4) commits. Will make this branch back to its original and focus on augmenting test tolerance tomorrow, if you leave some notes here about how to do that.

@strk
Copy link
Contributor Author

strk commented Dec 11, 2014

See http://hub.qgis.org/issues/11848 for possible cause of the failing tests.

@dakcarto
Copy link
Member

For instances where your labeling test images are close, but need some tolerance, you can add either color or mismatch tolerances to the base test, or add anomalies to the control image set.

From Modules section in labeling tests README:

def test_default_label(self):
   # Default label placement, with text size in points
   self._Mismatches['TestComposerPdfVsComposerPoint'] = 760
   self._ColorTols['TestComposerPdfVsComposerPoint'] = 18
   self.checkTest()

Values would replace the default values for the module or class, if any, for the
TestComposerPdfVsComposerPoint.test_default_label generated test.

@strk
Copy link
Contributor Author

strk commented Dec 11, 2014

Superceeded by squash-rebased version in #1731 -- @dakcarto will try your change there if it still fails

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants