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

Label pre-rotation #1757

Closed
wants to merge 4 commits into from
Closed

Label pre-rotation #1757

wants to merge 4 commits into from

Conversation

strk
Copy link
Contributor

@strk strk commented Jan 8, 2015

Pre-rotate vectors for placing labels.
This was suggested by @dakcarto in http://hub.qgis.org/issues/11814

Limiting pre-transformation to rotation requires later dropping rotation from the transform matrix used to get to device coordinates. This mixed approach would be fixed by directly acting at the device (pixel) level, as suggested in http://hub.qgis.org/issues/11856

\cc @dakcarto @wonder-sk

@strk
Copy link
Contributor Author

strk commented Jan 8, 2015

\cc @nirvn
Mathieu, note that with pre-rotation the block you recently changed is not needed anymore

Note for everybody: the need to change the matrix has been satisfied in an hacky way in this PR, with known memory leaks left in the code.

@@ -572,6 +578,12 @@ class CORE_EXPORT QgsGeometry
@param hasZValue 25D type?*/
void translateVertex( QgsWkbPtr &wkbPtr, double dx, double dy, bool hasZValue );

/**Rotate a single vertex around Z axis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment needs to be fixed, this is not restricted to "rotate" a vertex, but can be used for any affine transformation (could actually also replace the translateVertex method)

@nyalldawson
Copy link
Collaborator

@strk I can't comment on the label changes, but the geometry work looks good to me (requires sip bindings though, and inclusion of unit tests would be preferable). It will be very handy to have this rotate method available - I've needed something similar in the past. Thanks!

@nirvn
Copy link
Contributor

nirvn commented Jan 9, 2015

@strk nice! The more I was thinking of my hackish pull request which was create a situation where labels' distance from the labelled points was variable, the more uneasy I was becoming. This fixes it, and more.

@nirvn
Copy link
Contributor

nirvn commented Jan 9, 2015

@strk, @dakcarto I gave this pull request a try, and things are working good on my machine. The labels aren't disconnected from features anymore.

A zero-rotation area:
working-0

Same area, rotated 45 degrees, labels are straight and connected to features:
working

@nirvn
Copy link
Contributor

nirvn commented Jan 9, 2015

A more complex labelling scenario works flawlessly.

Zero-rotation:
complex-0

-55 degrees rotation:
complex

@strk
Copy link
Contributor Author

strk commented Jan 9, 2015

Thanks for testing, I'll be first pushing a commit with the QgsGeometry::rotate function, including SIP bindings and unit test, and then get back to the labeling part

@strk
Copy link
Contributor Author

strk commented Jan 9, 2015

e2e47d7 does the QgsGeometry::rotate part (and friends)

Revert custom rotation handling in label placement.
Drop rotation component from the transformation used to
determine placement as rotation was applied up-front.
@strk
Copy link
Contributor Author

strk commented Jan 9, 2015

squash-rebased, now looking at manually placed labels

@strk
Copy link
Contributor Author

strk commented Jan 9, 2015

The fact that manually-placed labels are defined by their upper-left corner makes for funny effects.
Could it make more sense to define label placement by their center ?

Sandro Santilli added 2 commits January 9, 2015 17:20
Also add a comment with the rationale for the need to reset rotation.
There is still another leak to fix, for the matrix set in the pal layer.
@strk
Copy link
Contributor Author

strk commented Jan 9, 2015

The code is now leak-less.

What I found is that often the PAL based label engine inappropriately uses the trick of computing the hortogonal distance between two corners of a rectangle to scale down a distance from map to device units. Of course when rotation is in charge hortogonal distance may be zero, messing up all computations. The last commit directly uses the "mapUnitsPerPixel" value instead.

@strk
Copy link
Contributor Author

strk commented Jan 9, 2015

Pushed as a09d3d6 and 9c6fe06

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.

None yet

3 participants