Add possibility do data define label position by point instead of X/Y #45007

domi4484 · 2021-09-09T16:52:11Z

Add possibility do data define label position by point instead of X/Y

This will allow to use the move label tool to change the position of a label coming from a point geometry column.
In the attached screencast a postgis table has a line geometry column and an additional point geometry column for the label position.

nyalldawson · 2021-09-09T19:19:09Z

Please don't merge before I've had a chance to review

m-kuhn · 2021-09-10T20:00:14Z

src/core/labeling/qgslabeling.h

@@ -30,6 +31,8 @@
 */
 class CORE_EXPORT QgsLabeling
 {
+    Q_GADGET


What is the Q_GADGET for?

To be able to use the Q_ENUM macro

Better approach is to move the new enum to the Qgis class -- that's the general direction we're heading now

Ok i can move it there

m-kuhn · 2021-09-10T22:00:04Z

src/core/geometry/qgsreferencedgeometry.cpp

@@ -66,3 +68,32 @@ QgsReferencedGeometry QgsReferencedGeometry::fromReferencedRect( const QgsRefere
  return QgsReferencedGeometry( QgsGeometry::fromRect( rectangle ), rectangle.crs() );
 }

+QgsReferencedGeometry QgsReferencedGeometry::fromEwkt( const QString &ewkt )


@nyalldawson FYI, I think that's the thing that's interesting in here

Ah yeah my comment was a mistake -- I didn't realise some of the changes were collapsed.

One question I do have with ewkt is if the crs number is a postgres srs id or is always an epsg code. The postgis docs suggest it's a postgis srid, which means the actual crs is server dependent and will vary server by server .. which means that interpretation has to reside in the data provider.

Is doing this in the provider not an incompatible change? We suddenly have different data types for columns (geometry or referenced geometry instead of string) and will break project configurations.

Right. I guess you'd need to ask the layer's data provider to convert the ewkt to a referenced geometry for you.

How would that be requested?

Not to forget, we also need a toEwkt method. Or support storing attributes with referenced geometry

Actually on second thoughts I'd avoid specifically referencing "ewkt" in the name, and go with "stringToReferencedGeometry" instead. Ewkt is postgis specific, but other providers may have an alternative format for string representation of geom + crs.

Something like virtual QgsReferencedGeometry QgsVectorDataProvider::fromEWkt( const QString& ewkt )? Returns null geometry by default, postgres provider can implement with correct crs retrieval.

These seems a cleaner solution to me, I'll go for that. Thank you both for your comments!

I was trying to move the functions to the data provider but I can't find a way to access the provider in QgsPalLabeling. Is it not available on purpose? How could i get it?

Hm, you're right. The PAL label registration methods can't have any access to the layer, as they'll be used in a background thread.

It's going to get super complex to handle this. I think the only way would be some small abstract "text to geometry" converter object which can be set in the render context, where you retrieve an instance of that from the layer's data provider and set it in the render context during the map render job preparation stages. (With care taken to make sure it's safe to do the conversion in a different thread from what this object was created in).

nyalldawson · 2021-09-14T20:14:31Z

@domi4484 I'd suggest in the interest of moving this forward that you split the work into two parts -- get the (basically unrelated) labelling part merged first, and then tackle the tricky ewkt handling in a follow up work.

On the ewkt note -- my impression is that ewkt has seen no adoption outside of postgis. I'm wondering if it wouldn't be simpler to ask the postgis team for a new "as ewkt2" method which uses the full wkt2 definition of the crs instead. That'd completely side step all the complexity then (and also be of more use as a quasi standard for use outside of postgis).

domi4484 · 2021-09-14T21:25:04Z

src/providers/postgres/qgspostgresprovider.cpp

+
+  // Look if crs already cached
+  QMutexLocker locker( mCrsCacheMutex );
+  if ( mCrsCache->contains( srid ) )


@nyalldawson I passed the crs cache to the converter object. In most cases the right crs will already be in the cache at the time of rendering labels.
Do you think this could be a good enough solution? Otherwise i will embrace your suggestion about splitting the pr

No, that doesn't help -- it uses a different definition of IDs. (And there's a chance a user will have two postgis connections with different crs definitions for the same id number)

Ok i understand, but in this case the current implementation is also wrong. The crs cache is static and not per provider

nirvn · 2021-09-15T02:05:01Z

Here's my two cents: instead of spending lots of energy on this ewkt thing, why don't we send it on fixing what's really broken here: the absence of geometry field support.

If the postgis layer column that holds that EWKT-stored geometry would be exposed in QGIS as a (referenced) geometry field, we wouldn't need EWKT acrobatics. The EWKT to geometry would happen within the provider's feature iterator.

nirvn · 2021-09-15T02:11:13Z

As for EWKT handling on the short term: users can come up with an expression which assumes the SRID ID == EPSG ID:

with_variable('bits', regexp_matches('SRID=4326;POINT(-44.3 60.1)', 'SRID=([0-9]+);(.+)'), transform(geom_from_wkt(@bits[1]), @bits[0], 'EPSG:4326'))

Obviously, the users would replace the arbitrary string I used above (i.e. SRID=4326;POINT(-44.3 60.1)) with their "field name".

nirvn · 2021-09-15T02:20:46Z

If someone wants to do a rock solid expression-based EWKT to geometry: he/she can actually add the postigs' spatial_ref_sys table in his/her project and query the SRID info by using get_feature_by_id('spatial_ref_sys',4326), which will give the auth_name and auth_srid attributes to get proper values that can be used by the transform function _in case SRID ID isn't a perfect match to the EPSG ID.

domi4484 · 2021-09-15T14:52:30Z

Here's my two cents here: instead of spending lots of energy on this ewkt thing, why don't we send it on fixing what's really broken here: the absence of geometry field support.

If the postgis layer column that holds that EWKT-stored geometry would be exposed in QGIS as a (referenced) geometry field, we wouldn't need EWKT acrobatics. The EWKT to geometry would happen within the provider's feature iterator.

Thank you @nirvn for taking us back to the root of the problem.
As suggested by @nyalldawson I started by shrinking down this pr removing again all the triky conversion thing.

At this state the limitation is that the label position read from an additional geometry field from postgis can't be parsed correctly (and this was the main objective of my work here).

Now I see three possible solution/workaround for this:

Quick ugly workaround in QgsPalLabeling. Check if the value strings starts with SRID=\d+; and chop it before passing it to QPoint::fromWkt. This will work if the additional geometry column as the same crs of the main geometry column.
Clean solution: a referenced geometry object value is used for additional geometry columns as for the "main" geometry column. (I can't estimate how many work is this and at how many places it will impact in qgis code)
Middle solution, during preparation of the rendering job pass a new QgsFeatureRequest::GeometryAttributes flag to getFeatures. The providers would take care of returning a wkt string for geometry columns.

My favorite at the moment is 1 but I don't know if it is acceptable :)

nirvn · 2021-09-16T01:39:15Z

-1 for the ugly workaround, +1 for the clean solution but I can see why it's not a welcoming option especially during feature freeze, -1 to introducing new flag.

Here's an alternative proposition: transform the EWKT string into a WKT string when fetching the column value in the provider (in QgsPostgresProvider::convertValue()). You can detect whether the field type name is "geometry", and if so transform EWKTs into WKT (re-projecting to the layer's CRS if needed).

That'd feel slightly less duck taped.

m-kuhn · 2021-09-16T05:52:15Z

If conversion of the attribute directly in the provider to a ReferencedGeometry is acceptable, this is certainly the cleanest option long term. I don't think inside qgis code it will require much changes.
It comes at a price though. Any expression that was written prior to QGIS 3.22 to parse existing ewkt will be broken. That's where a flag could help (middle option, but returning a ReferencedGeometry instead of wkt).

I think we either need to go for in-provider conversion, accepting the effects.
Or deal with the ewkt directly in the labeling (assuming the srid matches the epsg code). The advantage of this is that it's very low impact and will work quite well in many scenarios.
Or we decide it's not possible at all at the moment.

nirvn · 2021-09-16T06:34:04Z

@m-kuhn , ah right regarding the flag, makes sense.

nyalldawson · 2021-09-16T07:06:06Z

I'm +1 for changing to always returning referenced geometries, and making users deal with the fallout.

In fact, I started work on this yeeeeeears ago: https://github.com/nyalldawson/QGIS/commits/multi_geom2 😆

(Still a non trivial amount of work to do -- specifically handling the geometry values cleanly for updates/add features, and I lost interest as the project I was wanting this for wrapped up.)

github-actions · 2021-10-01T02:40:49Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

github-actions · 2021-10-09T02:39:06Z

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

github-actions · 2021-10-17T02:39:11Z

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

github-actions · 2021-11-06T02:40:07Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

m-kuhn · 2021-11-11T21:22:52Z

This one is still blocked by me

nyalldawson · 2021-12-02T01:00:34Z

@domi4484 thanks for picking this up again!

Coming back to the core topic of this change, I propose a slightly different UI for exposing this functionality. Instead of the drop down to switch between "X/Y" and "Point" modes, I'd instead suggest JUST having a new property override button sitting next to the existing X / Y ones which is "[.] Point". This can get automatically disabled if x or y is set (and vice versa -- the x/y buttons get disabled if point property is set).

It's a simpler UI, and would also substantially simplify the code (e.g. no need for the placementCoordinateType/setPlacementCoordinateType methods, since everything will be done via the property instead).

@nirvn would be keen for your thoughts regarding this proposal

nirvn · 2021-12-02T02:22:08Z

@nyalldawson , @domi4484 , sounds like a good solution to me, certainly better than the combobox and its addition getter/setter.

domi4484 · 2021-12-02T07:05:21Z

@nyalldawson good idea i will change it that way!

nyalldawson · 2021-12-02T07:06:43Z

@domi4484 are you happy for me to close this PR in the meantime? There's a lot of discussion here which is no longer relevant...

domi4484 · 2021-12-02T07:11:46Z

@nyalldawson yes no problem

github-actions bot added this to the 3.22.0 milestone Sep 9, 2021

m-kuhn reviewed Sep 10, 2021

View reviewed changes

domi4484 commented Sep 14, 2021

View reviewed changes

domi4484 added 6 commits September 15, 2021 15:05

switch to XY coordinate types when using label move tool

c110ca0

remove unneeded SIP_CONVERT_TO_SUBCLASS_CODE

2d39568

Add possibility do data define label position by point instead of X/Y

40972f2

added missing QRegularExpression include

739ed58

added mask image

1dd3473

Removed ewkt conversion functions

00f3b97

domi4484 force-pushed the labelPositioningPoint branch from 1c504b7 to 00f3b97 Compare September 15, 2021 13:55

Removed ewkt conversion test

d81362d

m-kuhn mentioned this pull request Sep 18, 2021

[postgis] Expose secondary geometry columns as referenced geometries #45127

Merged

github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 1, 2021

github-actions bot closed this Oct 9, 2021

m-kuhn reopened this Oct 9, 2021

github-actions bot closed this Oct 17, 2021

m-kuhn removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 17, 2021

m-kuhn reopened this Oct 17, 2021

nyalldawson added Frozen Feature freeze - Do not merge! and removed Frozen Feature freeze - Do not merge! labels Oct 21, 2021

github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 6, 2021

github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 11, 2021

m-kuhn modified the milestones: 3.22.0, 3.24 (Feature) Nov 22, 2021

Merge branch 'master' into labelPositioningPoint

0060fbd

m-kuhn closed this Dec 2, 2021

Add possibility do data define label position by point instead of X/Y #45007

Add possibility do data define label position by point instead of X/Y #45007

Conversation

domi4484 commented Sep 9, 2021

Add possibility do data define label position by point instead of X/Y

nyalldawson commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyalldawson commented Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

nyalldawson Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirvn commented Sep 15, 2021 • edited Loading

nirvn commented Sep 15, 2021

nirvn commented Sep 15, 2021 • edited Loading

domi4484 commented Sep 15, 2021

nirvn commented Sep 16, 2021

m-kuhn commented Sep 16, 2021

nirvn commented Sep 16, 2021

nyalldawson commented Sep 16, 2021

github-actions bot commented Oct 1, 2021

github-actions bot commented Oct 9, 2021

github-actions bot commented Oct 17, 2021

github-actions bot commented Nov 6, 2021

m-kuhn commented Nov 11, 2021

nyalldawson commented Dec 2, 2021

nirvn commented Dec 2, 2021

domi4484 commented Dec 2, 2021

nyalldawson commented Dec 2, 2021

domi4484 commented Dec 2, 2021

nyalldawson commented Sep 14, 2021 •

edited

Loading

nyalldawson Sep 14, 2021 •

edited

Loading

nirvn commented Sep 15, 2021 •

edited

Loading

nirvn commented Sep 15, 2021 •

edited

Loading