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.

Sign up for GitHub

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

Jump to bottom

Viewport rotation support in composer #1882

Closed
wants to merge 1 commit into from
Closed

Viewport rotation support in composer #1882

wants to merge 1 commit into from

Conversation

strk
Copy link
Contributor

@strk strk commented Feb 4, 2015

This is a work in progress, see
http://hub.qgis.org/issues/11912

@nyalldawson nyalldawson self-assigned this Feb 4, 2015
@nyalldawson
Copy link
Collaborator

@strk just a note - the code in QgsComposerMap is extremely fragile, so please don't merge any changes before I've had a chance to review them

@strk
Copy link
Contributor Author

strk commented Feb 4, 2015

@nyalldawson I don't plan to merge this before a deep review and an approval.
With latest commit the position of a rotated map seems fine to me, could you please take a look ?
\cc @nirvn

@nirvn
Copy link
Contributor

nirvn commented Feb 5, 2015

@strk @nyalldawson , I've built and tested this PR and confirm that the rotation works fine, as well as grid and overview who now align flawlessly:

working

IMO, this should be merged before the release of 2.8; I've always considered the lack of implementation within the composer as an implementation gap (which is considered a blocker).

@gioman
Copy link
Contributor

gioman commented Feb 5, 2015

If you plan to merge this do it as soon as possible, to allow us do extensive tests.

@strk
Copy link
Contributor Author

strk commented Feb 5, 2015

Travis is failing, I guess some tests would need to be changed to expect the now-correct behavior.
Also, I'd like a word from the release manager about putting this in master. \cc @jef-n

@strk
Copy link
Contributor Author

strk commented Feb 5, 2015

The travis failures are here: http://dash.orfeo-toolbox.org/testDetails.php?test=28184445&build=170932

That test is particularly unfortunate in that it's using a pretty noisy raster which makes comparing expected/obtained harder than it could be. Ideally the raster would be a neatly defined 2-colors kind of thing...

@nirvn
Copy link
Contributor

nirvn commented Feb 5, 2015

I gave it further testing this afternoon (testing it out in the context of an atlas export, adding an overview onto a rotated map, etc.), things are looking good. Used two large projects too, couldn't see any issues.

@strk
Copy link
Contributor Author

strk commented Feb 5, 2015

If anyone can help with the failing testcases my branch is open for pull requests - https://github.com/strk/qgis/pulls

@strk
Copy link
Contributor Author

strk commented Feb 5, 2015

See #1884 for the direction I have in mind for the tests...

@strk
Copy link
Contributor Author

strk commented Feb 5, 2015

I've rebased to master after simplifying the test raster. Hopefully now we can see better what fails and how different the results are from the expected (debatable expectances when it comes to center, btw)

@strk
Copy link
Contributor Author

strk commented Feb 5, 2015

Great, failures seem tolerable:
http://dash.orfeo-toolbox.org/testDetails.php?test=28205994&build=171052
It may be better to use a tolerance this time, rather than an "anomaly"

As for the second failure, I think it may be a completely unrelated race condition (presence of the old image in the second test): http://dash.orfeo-toolbox.org/testDetails.php?test=28205995&build=171052

@strk
Copy link
Contributor Author

strk commented Feb 5, 2015

See ? now the second test did not fail. Maybe the QVERIFY failure avoids to run the subsequent code, which includes a "removeItem"...

In any case, this PR is now succeeding. Anything preventing a merge to master ?

@nyalldawson
Copy link
Collaborator

@strk shouldn't be. Give me 48 hours or so and I'll review and merge. Thanks!

@wonder-sk
Copy link
Member

@strk I think it would be very useful to move the code that calculates polygon of the rotated extent to QgsMapSettings class - the visibleExtent() method only returns bounding box of the rotated extent...

@strk
Copy link
Contributor Author

strk commented Feb 6, 2015

@wonder-sk I think I'm going to do that as part of the change that fixes the overview map.
Here I think the work can be completed by removing warnings and squashing to master, which I'll be doing now.

@strk
Copy link
Contributor Author

strk commented Feb 6, 2015

@wonder-sk PR #1888 has the QgsMapSettings method to get possibly rotated map extent, being a new interface I dunno if it's ok to push to master (although completely safe).

@strk strk force-pushed the master-rotate-composer branch 2 times, most recently from eab95d0 to 5c3623d Compare February 6, 2015 11:11
@strk
Copy link
Contributor Author

strk commented Feb 6, 2015

I've squashed the composer commits and rebased on master + visiblePolygon PR (#1888).

@strk
Copy link
Contributor Author

strk commented Feb 6, 2015

Sorry, rebased again, the "visiblePolygon" interface is not really needed for this PR and I don't want to mix things. Better to merge this separately from that, as long as all tests still succeed.

nyalldawson
nyalldawson reviewed Feb 7, 2015
View reviewed changes
src/core/composer/qgscomposermap.cpp
@@ -2055,7 +2032,7 @@ QString QgsComposerMap::displayName() const
void QgsComposerMap::requestedExtent( QgsRectangle& extent ) const
{
QgsRectangle newExtent = *currentMapExtent();
if ( mEvaluatedMapRotation == 0 )
if ( 1 || mEvaluatedMapRotation == 0 )
Copy link
Collaborator

Choose a reason for hiding this comment

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

@strk I don't think this change should be included. It breaks API and affects QgsComposerLegend::updateFilterByMap() potentially causing features to be missed in a legend filtered from a rotated map (although the current behaviour isn't ideal as it potentially includes features which aren't visible, but that's safer then the alternative). Instead, the calls to requestedExtent in cache and paint should be replaced by *currentMapExtent() instead.

@nyalldawson
Copy link
Collaborator

@strk looks good to me, apart from the note about QgsComposerMap::requestedExtent breakage. I'm happy for it to be merged otherwise.

Small tip - including "fix #11912" in the commit message will auto-close the bug in redmine.

@strk
Copy link
Contributor Author

strk commented Feb 7, 2015

@nyalldawson could you add a testcase guarding the API for QgsComposerMap::requestedExtent ?
Or suggest one ? I'll work on reverting that change and changing the callers instead.

@strk
Copy link
Contributor Author

strk commented Feb 7, 2015

There are multiple callers but I can't find a way to trigger all of them. For example, how do I set "plotStyle" to "preview" ?

@nyalldawson
Copy link
Collaborator

@strk it's set by the composition - a composition is in preview mode in the GUI, and print/postscript if it's bring exported.

@strk
Copy link
Contributor Author

strk commented Feb 7, 2015

I've pushed a fix of the regression you pointed out. Please test again.
Once happy I'll squash-rebase and add the fix #11912 label

@strk
Copy link
Contributor Author

strk commented Feb 7, 2015

Pushed as a40eca4

@strk strk closed this Feb 7, 2015
@strk strk deleted the master-rotate-composer branch February 7, 2015 14:35
@qgib qgib mentioned this pull request May 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nyalldawson nyalldawson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@strk @nyalldawson @nirvn @gioman @wonder-sk