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

Template based referenced geometry class #4720

Merged
merged 8 commits into from
Sep 6, 2017

Conversation

nyalldawson
Copy link
Collaborator

@nyalldawson nyalldawson commented Jun 13, 2017

This PR adds a new template based QgsReferencedGeometryPrimitive class, which consists of a geometry primitive and an associated CRS. The intention here is that primitive specific classes are created - included in this PR is QgsReferencedRectangle, for a QgsRectangle with optional CRS. But other uses could be referenced QgsPointXY, QgsGeometry, QgsCircle, etc....

Use is:

referenced_rect = QgsReferencedRectangle( QgsRectangle(1,2,3,4), QgsCoordinateReferenceSystem('epsg:4326') )
print(referenced_rect.rect().width())
print(referenced_rect.crs().authid())

I think this is a better approach then subclassing QgsRectangle, because:

  1. subclassing QgsRectangle would add a virtual pointer to QgsRectangle, increasing it's memory requirements (which has a flow on to all QgsAbstractGeometry objects which cache the bounding box)
  2. we'd run the risk of copies slicing away the crs information... so we'd need to add a clone() method to QgsRectangle and majorly complicate its api.

An alternative approach would be adding the crs as a member to QgsRectangle - but again there's a memory cost here and QgsRectangle needs to be kept small and light.

That's why I've gone for this approach. I don't like having to access the underlying rectangle with the .rect() method, but I can't see a good alternative.

Main use case here is:

  • metatyping QgsReferencedRectangle
  • allow processing extent parameters to accept QgsReferencedRectangle QVariant values, so that we know whether we need to reproject the rect or geometries so that they can be used with each other

@3nids
Copy link
Member

3nids commented Jun 13, 2017

The script wasn't waiting for spaces around the <...>, fixed.
I pushed the updated sip file to your branch, but I did not rebased to avoid forced push.
Let's wait on Travis, I could not get QgsRange to use the template approach, and I don't understand why. Let's see here!

@3nids 3nids force-pushed the ref_geom branch 4 times, most recently from 30ec81b to 1cf53ff Compare June 13, 2017 09:43
@3nids
Copy link
Member

3nids commented Jun 13, 2017

LGTM!

@nyalldawson
Copy link
Collaborator Author

@m-kuhn @wonder-sk keen for feedback here

@wonder-sk
Copy link
Member

I would probably still reconsider the approach with having QgsReferencedRectangle as a subclass of QgsRectangle:

  • we do not have to have extra vtable in QgsReferencedRectangle if we do not add any virtual functions - which I think we do not need/want here anyway
  • I do not see any trouble with slicing away CRS information when copying... if we want to copy just coordinates and skip the CRS part, I think this is where slicing works to our advantage.

The only source of trouble I can see (in the scenario without virtual destructor) is when someone would be passing QgsReferencedRectangle by pointer, then it would be cast to QgsRectangle pointer and then deleted using that pointer, but this sounds like unlikely scenario as rectangles are normally meant to be passed by reference or by value.

@m-kuhn
Copy link
Member

m-kuhn commented Jun 15, 2017

What's the plan with?

QgsRectangle intersect (const QgsRectangle *rect) const

Hide this in the subclass?

QgsReferencedRectangle intersect(const QgsRectangle *rect) const

On the other hand, the composition approach in this pull request could be enhanced with a conversion operator, no need to explicitly convert every time (in C++ at least).

operator T() const { return mPrimitive; }

@wonder-sk
Copy link
Member

QgsRectangle intersect (const QgsRectangle *rect) const

should be IMHO changed to

QgsRectangle intersect (const QgsRectangle &rect) const

(no reason to pass by pointer)

and in the referenced rect subclass there could be extra override:

QgsReferencedRectangle intersect (const QgsReferencedRectangle &rect, bool *ok = 0) const

(the extra "ok" output argument to indicate reprojection errors)

So if at least one of the rects is not referenced, you get the basic variant thanks to automatic upcasting, when they are both referenced rects, you get the variant with reprojection...

@nyalldawson
Copy link
Collaborator Author

@wonder-sk hows this approach for you?

@wonder-sk
Copy link
Member

image
Yes I like.

@nyalldawson
Copy link
Collaborator Author

Travis is consistently timing out at the moment, but this passed before I fixed a trival merge conflict. So I'm declaring it safe to merge.

@nyalldawson nyalldawson merged commit 2e20b6f into qgis:master Sep 6, 2017
@nyalldawson nyalldawson deleted the ref_geom branch September 6, 2017 03:51
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

4 participants