QgsPoint vs QgsPointV2 ?

[mhugo]

I find confusing to have both QgsPoint and QgsPointV2. The second can carry Z/M values, but not the first. And lots of methods are using / returning QgsPoint where they should use/return QgsPointV2 because Z/M values could have a meaning. I would be in favor of having only one class with full Z/M support

[nyalldawson]

Actually, I think we still have a need for a non-QgsPointV2 class, just for storing simple x/y values. QgsPointV2 is quite heavy. QgsPoint has a lot of unnecessary stuff (and duplicates a lot of QgsPointV2). What we need is a version of QPointF which is guaranteed to always store doubles.

@wonder-sk and I have spoken briefly about this - maybe QgsXY would be a good name.

[mhugo]

QgsPointV2 is quite heavy

You mean because it also stores Z and M ? (and has a vtable)

[wonder-sk]

You mean because it also stores Z and M ? (and has a vtable)

... and bounding box and wkb type :)

sizeof(QgsPoint) == 16 vs sizeof(QgsPointV2) == 80

That's quite a lot of overhead if you just want to store a point. Sure, bounding box and wkb type may be removed from base class (and I hope they will be), but still 95% of time you just want to work with X,Y coords...

[anitagraser]

There are certain functions, such as azimuth(), which are currently only available in QgsPoint but not in QgsPointV2. Will these be made available or is there a convenient way to convert a V2 to V1?

[m-kuhn]

I would keep it as-is and not rename QgsPoint.

What I find confusing is that we now have some Z/M geometries with V2 suffix while others have it removed. I think we could revert the name QgsLineString to QgsLineStringV2 for consistency. Or replace the V2 suffix with a ZM suffix which is semantically clearer.

[nyalldawson]

I'd prefer to go the other way and change the names of the other classes - ie QgsPolygon to QgsRingSequence and QgsMultiPoint to QgsPointList.

I think it's very confusing having these list classes (QgsPolygon/QgsMultiPoint/QgsPolyline), and their use should be heavily discouraged as using them drops z/m values. It's not obvious for scripters that this is going to happen, yet I see asPoint(), asPolyline(), asPolygon() used everywhere (even in QGIS core + inbuilt plugins like processing).

Giving them obscure sounding names like QgsRingSequence is one step towards avoiding their use ;)

[nyalldawson]

There are certain functions, such as azimuth(), which are currently only available in QgsPoint but not in QgsPointV2. Will these be made available or is there a convenient way to convert a V2 to V1?

I've ported some in https://github.com/qgis/QGIS/commit/983fe24806b47d4c8a9aa6d226685be3fb8de90f

[nyalldawson]

So what's the plan of attack here? Can we make a decision?

[wonder-sk]

To me QgsPointV2 should be renamed for QgsPoint - for consistency, and QgsPoint should be renamed to something else (QgsXY, QgsPoint2D, ...)

For asPoint(), asPolyline(), asPolygon() and friends, I would suggest that we introduce replacements in their respective QgsAbstractGeometry classes - ones that would also keep Z/M coords. I would also add various interators to the geometry classes (e.g. for vertices, rings, parts), so we can get rid of some awkward bits in the geometry classes (like coordinateSequence() returning list-of-list-of-list-of-points, nextVertex() implementing something like iterator) and clean up the accessors (e.g. in QgsLineString: points() / pointAt() / xAt() / yAt() / vertexAt() in addition to coordinateSequence() and nextVertex()).

[m-kuhn]

It's consistent, making the Z/M classes default sounds good and having foreach and friends work on the geometries sounds even better. +1

[NathanW2]

+1 for default Z/M support.

On Mon, Nov 14, 2016 at 6:20 PM, Matthias Kuhn notifications@github.com wrote:

It's consistent, making the Z/M classes default sounds good and having foreach and friends work on the geometries sounds even better. +1

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/qgis/qgis3.0_api/issues/36#issuecomment-260274151, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXS3A_yV9KtiAoQS6eWSk-5z9qxGknzks5q-BnggaJpZM4HJWJx .

[vmora]

@nyalldawson, @wonder-sk what about a compromise with sizeof(QgsPoint)=24 ?

What I'm thinking about is to use strong typing instead of the member variable mWkbType in QgsGeometry:

• QgsPointZ, QgsPointM and QgsPointZM would be templated and inherit QgsPoint (the current V2 one)
• other types, which are lists would be templated on QgsWkbTypes::Type

With this approach, we could get rid of QgsXY or whatever (at the expense of 30% in memory footprint to store the vtable), and keep the storage minimal for all geometries.

[m-kuhn]

What do you think about the name QgsPointXY? I don't like it too much actually.

I would be glad to get another input on this. I was thinking about calling it QgsPoint2d instead. I don't like QgsXY too much either.

@nyalldawson @wonder-sk @3nids @NathanW2 @mhugo

PS: It should be quite easy to update this to a different name now. I was in a hurry to get the QgsPointV2 > QgsPoint change merged quickly because of obvious frequent rebasing issues.

[3nids]

I'm fine with QgsPoint2D. QgsSimplePoint ? QgsBasicPoint?

[wonder-sk]

I would prefer QgsPoint2D as well. It is also in line with naming of Qt classes, e.g. QVector2D, QVector3D.

[nyalldawson]

Would we go QgsPoint2d or QgsPoint2D? I'd lean to 2d given we are removing all other capitalised non first letters

[m-kuhn]

tough decision!

[wonder-sk]

I would lean towards 2D :-) with 3D support in QGIS there will be various bits in API where we would have various uses of e.g. QVector3D so having QgsPoint2d would look a bit odd...

We can always roll dice ;-)

[m-kuhn]

We followed Qt for caps in abbreviations. Let's just do the same here.

QgsPoint2D it is :)

[nyalldawson]

Hmmm. Isn't 2d an acronym? How does it differ from XML?

[m-kuhn]

In that the first "letter" isn't a letter but a number that cannot be capitalized :P

Consistency with Qt sounds like a VERY good argument to me.

[nyalldawson]

Yeah, I'm just stirring things up. It's inconsistent and grates on me, but we'll add it to the list of things to blame on qt ;)

[nyalldawson]

This can be closed now, right?