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

Symbology brush transform #2666

Merged
merged 12 commits into from
Jan 19, 2016
Merged

Conversation

mhugent
Copy link
Contributor

@mhugent mhugent commented Jan 12, 2016

This pull request adds a new render flag 'RenderMapTile'. The idea is that if setting this in a project, QGIS knows that a map tile is generated for use in a tile cache. E.g. with the current pull request, QGIS makes sure that image fills between adjacent tiles match if the flag is set (it uses a brush transformation in QgsImageFillSymbolLayer). In future, there are other things which could be considered for tile image rendering. E.g. some wms servers don't place labels into the border zone if images for tile caches are rendered.

A unit test is provided too. The test assembles four adjacent tiles to one image and compares against a reference image.

…oint if rendering a map tile. Therefore it is assured, that the image pattern matches for adjacent tiles of the same scale
@mhugent mhugent assigned mhugent and nyalldawson and unassigned mhugent Jan 12, 2016
@nirvn
Copy link
Contributor

nirvn commented Jan 12, 2016

@mhugent that sounds exciting. Do you have any screenshots at hand demonstrating the positive impact of this PR?

@mhugent
Copy link
Contributor Author

mhugent commented Jan 12, 2016

@nirvn: there are two test images in this pull request, each composed of four part images. Without the PR, there would be discontinuities at the borders of the part images.

//transform brush to upper left corner of geometry bbox
QPointF leftCorner = points.boundingRect().topLeft();
QTransform leftCornerTransform = QTransform::fromTranslate( leftCorner.x(), leftCorner.y() );
mBrush.setTransform( leftCornerTransform );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like in some places a transform is already set on the brush. Might be safer to take the existing brush transform and then translate it (similar to how rotation is done in line 1593)

@nyalldawson
Copy link
Collaborator

Looks good to me! just the few minor things noted above + failing tests to be addressed.

@@ -50,7 +50,7 @@ class QgsVectorLayerDiagramProvider;
* @note added in 2.4
* @note not available in Python bindings
*/
class QgsVectorLayerRenderer : public QgsMapLayerRenderer
class CORE_EXPORT QgsVectorLayerRenderer : public QgsMapLayerRenderer
Copy link
Member

Choose a reason for hiding this comment

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

Why to export the class in qgis_core library? it was not meant for that... I see the class is used in the unit test but it is an implementation detail that QgsVectorLayerRenderer exists and unit tests should not use it....

@nyalldawson
Copy link
Collaborator

@mhugent - to get the test to pass under OSX you'll need to use the scripts/generate_test_mask_image.py script.

Eg - find failure result on dash (http://dash.orfeo-toolbox.org/testDetails.php?test=36433187&build=214498), then run

scripts/generate_test_mask_image.py testFourAdjacentTiles1 http://dash.orfeo-toolbox.org/displayImage.php?imgid=71581

(where http://dash.orfeo-toolbox.org/displayImage.php?imgid=71581 is the url for the rendered image)

This will create a new "_mask.png" file alongside the test reference image which will give the test tolerance respecting that mask.

Not sure why the sip build is failing though - maybe it's the missing enum values?

@@ -276,6 +276,9 @@ class QgsMapCanvas : QGraphicsView
//! true if antialising is enabled
bool antiAliasingEnabled() const;

//! sets map tile rendering flag
void enableMapTileRendering( bool theFlag );
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be 'setMapTileRendering' for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind if 'set' or 'enable'. Just thought enableMapTileRendering would be consistent with enableAntiAliasing.

@nyalldawson
Copy link
Collaborator

@mhugent I was going to say - if you want to get this in before freeze maybe #ifdef out the tests for now, and we'll sort them out later

@mhugent
Copy link
Contributor Author

mhugent commented Jan 15, 2016

Now it succeeds on Mac but not on Linux. The error message:

[ 95%] Building CXX object python/CMakeFiles/python_module_qgis__gui.dir/gui/sip_guipart0.cpp.o

clang: error: unable to execute command: Killed

is not really revealing. Is that a timeout or something?

@nyalldawson
Copy link
Collaborator

Hmmm... I can't see any reason for that at all. @jef-n @m-kuhn any ideas?

@mhugent mhugent added this to the 2.14 milestone Jan 15, 2016
@timlinux
Copy link
Member

+1 from me to see this merged even though the freeze window for 2.14 is closed! Thanks for doing the @mhugent - tiling with current QGIS releases is a pain because of the issues you mentioned.

@mhugent
Copy link
Contributor Author

mhugent commented Jan 18, 2016

This PR can also be seen as some sort of bug fix, so feature freeze might not be a problem.
Still, I have no idea, why the changes do not work with sip on the Linux test machine.

@m-kuhn
Copy link
Member

m-kuhn commented Jan 18, 2016

Compiles fine for me on Fedora linux and I can't see where the source of the problem is.

I'd say revert all sip file changes and re-enable them one-by-one to find the culprit?

@mhugent
Copy link
Contributor Author

mhugent commented Jan 18, 2016

@m-kuhn , @nyalldawson : looks like the linux build now already fails when creating the build environment.

@mhugent
Copy link
Contributor Author

mhugent commented Jan 18, 2016

Same sip problems even if no sipfile was changed

@nyalldawson nyalldawson mentioned this pull request Jan 18, 2016
@nyalldawson
Copy link
Collaborator

@mhugent can you rebase, readd the sip bindings, and force push? Hopefully it's fixed now.

mhugent added a commit that referenced this pull request Jan 19, 2016
@mhugent mhugent merged commit f4bdaad into qgis:master Jan 19, 2016
@mhugent
Copy link
Contributor Author

mhugent commented Jan 19, 2016

Worked. Thanks, @nyalldawson !

nyalldawson added a commit to nyalldawson/QGIS that referenced this pull request May 19, 2016
Implements the improved mouse/key modifier behaviour discussed in:
http://osgeo-org.1560.x6.nabble.com/Key-modifiers-with-selection-tc5239653.html

Specifically,

For click-and-drag selections:
- holding shift = add to selection
- holding ctrl = substract from selection
- holding ctrl+shift = intersect with current selection
- holding alt (can be used with shift/ctrl too) = change from
"intersects" to "fully contains" selection mode

For single-click selections:
- holding shift or ctrl = toggle whether feature is selected
(ie either add to current selection or remove from current
selection)

This brings the canvas behaviour into line with other design apps
and also with the composer behaviour.

(fix qgis#2666)
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.

6 participants