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

postgres provider: allow to load layers with polyhedral surfaces or TINs #2322

Merged
merged 1 commit into from
Oct 21, 2015
Merged

postgres provider: allow to load layers with polyhedral surfaces or TINs #2322

merged 1 commit into from
Oct 21, 2015

Conversation

mhugo
Copy link

@mhugo mhugo commented Sep 17, 2015

This restores a hack that was introduced prior to geometry refactoring in order to be able to load TIN and polyhedral surfaces layers in QGIS (in 2D).

@jef-n @mhugent just to be sure it does not break anything else ...

Would there be any way to setup a postgis provider unit tests ?

@m-kuhn
Copy link
Member

m-kuhn commented Sep 17, 2015

like this? https://github.com/qgis/QGIS/blob/master/tests/src/python/test_provider_postgres.py

@strk
Copy link
Contributor

strk commented Sep 17, 2015

is TRIANGLE already supported ? that's also an areal geometry

@strk
Copy link
Contributor

strk commented Sep 17, 2015

about that test: it might be useful for it to allow changing connection parameters via an env variable (libpq standard variables could do, beside database name) -- is that test already run on "make check" ?

@mhugo
Copy link
Author

mhugo commented Sep 17, 2015

@m-kuhn ah ! sorry I missed it :) I've just realised there is also a dedicated compilation option. Perfect. I'll have a look at it, thanks.

@strk you're right, TRIANGLE should also be handled. And I agree that the db parameters should be customizable

@mhugo mhugo force-pushed the fix_tin branch 2 times, most recently from 4d479b8 to fefe94d Compare September 18, 2015 13:48
@m-kuhn m-kuhn added Requires Changes! Waiting on the submitter to make requested changes Requires Tests! Waiting on the submitter to add unit tests before eligible for merging labels Sep 21, 2015
@mhugo
Copy link
Author

mhugo commented Sep 24, 2015

@m-kuhn Is the postgres provider test is run during a pull request build ? It should fail since it now relies on new tables ... which should not be there without your manual intervention. Right ?

@m-kuhn
Copy link
Member

m-kuhn commented Sep 24, 2015

@mhugo yes, they are run during pr builds. but since the db is recreated on every travis build, you can add your own extensions ( see https://github.com/qgis/QGIS/blob/master/ci/travis/linux/before_script.sh ) without my intervention.

@mhugo
Copy link
Author

mhugo commented Sep 25, 2015

@m-kuhn ah nice! So my new tests have successfully passed :)

@mhugo
Copy link
Author

mhugo commented Sep 25, 2015

@m-kuhn You added "Requires tests". Do you think this requires additional tests ? Or is it ok to merge before the freeze ?

@m-kuhn
Copy link
Member

m-kuhn commented Sep 25, 2015

Thanks for the tests.

Assigned @jef-n , he's most experienced with the postgres provider I think.

@m-kuhn m-kuhn added For Review and removed Requires Changes! Waiting on the submitter to make requested changes Requires Tests! Waiting on the submitter to add unit tests before eligible for merging labels Sep 25, 2015
PostGIS: Allow to load TIN, PS and Triangle layers
c7aeb77 
The postgres provider is modified so that layers with
TIN, PolyhedralSurface and Triangle geometries can be loaded.
Geometries are converted to MultiPolygons (and Polygons for Triangles).

The postgres test is completed to cover the loading of different types
of layers
@mhugo
Copy link
Author

mhugo commented Oct 21, 2015

Back on this topic.
This new version does not touch the geometry types, only the postgres provider. I think it's better, no new wkb type that are not really supported. It also fixes a part of the code in featureiterator that is now useless for most of the 2.5D geometries (only needed for TIN). And unit tests are now present.
Since it fixes a regression from 2.8, I'll merge it.

mhugo pushed a commit that referenced this pull request Oct 21, 2015
Merge pull request #2322 from mhugo/fix_tin
a460e47 
Postgres provider: allow to load layers with polyhedral surfaces or TINs
@mhugo mhugo merged commit a460e47 into qgis:master Oct 21, 2015
@mhugo mhugo deleted the fix_tin branch March 18, 2016 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jef-n jef-n

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mhugo @m-kuhn @strk @nyalldawson @jef-n