Geo CRS handling by yarox-1 · Pull Request #5 · ad-freiburg/util

yarox-1 · 2026-07-10T12:50:08Z

Transformations between different CRS (=coordinate reference system).

patrickbr · 2026-07-13T10:17:03Z

Thank you for this! I know this is still a draft, but we should discuss whether we really want to store the CRS within the Point class. I personally do not think that this is a good idea. I know that this is only an additional 8-bit integer atm, but there are more than 256 projections, and even with only 8 bit a lot of our existing tools (for example petrimaps) which work on 16 bit or 32 bit geometries internally would suddenly need 12,5% (32bit) or 25% (16 bit) more memory.

It would also allow strange things like lines and polygons which consist of points of different CRS (in the case of line and polygons, storing the same projection <number of points> times is also extremely redundant).

Maybe we could first clarify where we would actually need this?

I think a sane approach would be to have the CRS as an optional parsing result (you already implemented this), to enable automatic conversion from input projections other than WGS84. There should of course also be a single internal reference projection (we currently use Web Mercator) to which the point is reprojected on parsng, not a re-projection of points on-the-fly on every access.

yarox-1 · 2026-07-14T08:23:01Z

Thank you for this! I know this is still a draft, but we should discuss whether we really want to store the CRS within the Point class. I personally do not think that this is a good idea. I know that this is only an additional 8-bit integer atm, but there are more than 256 projections, and even with only 8 bit a lot of our existing tools (for example petrimaps) which work on 16 bit or 32 bit geometries internally would suddenly need 12,5% (32bit) or 25% (16 bit) more memory.

It would also allow strange things like lines and polygons which consist of points of different CRS (in the case of line and polygons, storing the same projection times is also extremely redundant).

Thanks for the review. I totally get your point. I did not know of the impact on the existing tools. I will remove the CRS type from the points.

Maybe we could first clarify where we would actually need this?

I dont know the whole scope yet. The main idea would be to allow queries with CRS IRIs in front of geometries. Later geof:transform should also work with different CRS IRIs. I'm trying to solve these two QLever issues:

ad-freiburg/qlever#2935
ad-freiburg/qlever#1797

I think a sane approach would be to have the CRS as an optional parsing result (you already implemented this), to enable automatic conversion from input projections other than WGS84. There should of course also be a single internal reference projection (we currently use Web Mercator) to which the point is reprojected on parsng, not a re-projection of points on-the-fly on every access.

I also think that this sounds like a good idea. Do I understand it correctly that during parsing all geometries should be transformed to WebMercator? I previously thought CRS84 (LngLat) was used.

patrickbr · 2026-07-16T14:09:29Z

I did not know of the impact on the existing tools. I will remove the CRS type from the points.

Nice, thank you :)

I dont know the whole scope yet. The main idea would be to allow queries with CRS IRIs in front of geometries. Later geof:transform should also work with different CRS IRIs.

geof:transform does not require the source geometry to store its projection if we know that the projection is guaranteed to be some fixed internal projection.

I also think that this sounds like a good idea. Do I understand it correctly that during parsing all geometries should be transformed to WebMercator? I previously thought CRS84 (LngLat) was used.

It's a bit misleading to say what is used internally because it may mean different things, so my remark was a bit confusing, sorry. spatialjoin uses Web Mercator internally, folded points in QLever somewhat assume WGS84 internally, but really just store "dumb" floating point numbers. On the QLever-side, I would say that the easiest way to support different CSR would be to treat this as a parsing problem and parse any CRS into WGS84. This would not break any existing code (WGS84 is assumed by default everywhere at the moment). Everything would continue to work, and geof:transform may also just assume that the input is given in WGS84.

patrickbr · 2026-07-16T14:23:51Z

By the way: storing the original projection explicitly would be necessary for the folded points in QLever, I think. Otherwise we would loose the ability to output the original input WKT of a point. But this is something that should be done in QLever, not here. @ullingerc can give you better guidance on that :)

ullingerc · 2026-07-16T14:35:54Z

@patrickbr Thanks for your comments. This is part of Yannik's bachelor thesis we talked about. I'll discuss details in a meeting with Yannik tomorrow and get back to you as soon as this PR is ready from my point of view. And I totally aggree that (a) we need to know the original CRS for some of the use cases, (b) the CRS should not be inside the Point class.

…rsion to CRS84 default)

ullingerc

Thanks for these changes.
A first pass on everything.

ullingerc · 2026-07-22T12:45:52Z

+  PLACEHOLDER1 = 4,
+  PLACEHOLDER2 = 5,
+  PLACEHOLDER3 = 6,
+  PLACEHOLDER4 = 7


What are these "placeholder"s for?

I just left them as placeholder for later adding other systems/IRIs. Currently they have no use.

Remove these placeholders please. They are not needed and the enum can be extended anytime in the future.

ullingerc · 2026-07-22T12:59:17Z

+    if constexpr(InvocableWithExactReturnType<F, Point<T>, const Point<double>&>) {
+      // If the 'projFunc' does not take CRS types into account, transform to default 'CRS84'.
+      line.push_back(projFunc(convertToCRS84(util::geo::DPoint(x, y), sourceCRS)));
+    } else {
+      static_assert(InvocableWithExactReturnType<F, Point<T>, const Point<double>&, CRSType>);
+      line.push_back(projFunc(util::geo::DPoint(x, y), sourceCRS));
+    }


This code is redundant and we should implemented only once. The important part is that this is properly templated s.t. inlining still works.

Something like

template <typename F, typename T> decltype(auto) applyProj(F&& projFunc, T x, T y, const CRSType& sourceCRS) { if constexpr (InvocableWithExactReturnType<F, Point<T>, const Point<double>&>) { // projFunc doesn't take CRS into account, transform to default CRS84 first. return projFunc(convertToCRS84(util::geo::DPoint(x, y), sourceCRS)); } else { static_assert(InvocableWithExactReturnType<F, Point<T>, const Point<double>&, CRSType>); return projFunc(util::geo::DPoint(x, y), sourceCRS); } } // and then: line.push_back(applyProj(projFunc, x, y, sourceCRS));

Also you may explore adding [[always_inline]] to applyProj and the projection functions themselves.

Heads up: please read @patrickbr's comments below before implementing. This should be done differently...

ullingerc · 2026-07-22T13:02:35Z

+project(util_standalone_test CXX)
+enable_testing()
+# -----
+


what's up with this?

This should not be part of the finalized PR. This was needed for me to enable testing.

patrickbr · 2026-07-22T13:33:42Z

+    if constexpr(InvocableWithExactReturnType<F, Point<T>, const Point<double>&>) {
+      // If the 'projFunc' does not take CRS types into account, transform to default 'CRS84'.
+      line.push_back(projFunc(convertToCRS84(util::geo::DPoint(x, y), sourceCRS)));
+    } else {
+      static_assert(InvocableWithExactReturnType<F, Point<T>, const Point<double>&, CRSType>);
+      line.push_back(projFunc(util::geo::DPoint(x, y), sourceCRS));
+    }


Note that there are many projects using this lib which are still on C++14 (or even C++11. So I would prefer it if we avoided std::is_invocable et al.

Could you give a motivation for supporting two types of projFunc? (one which accepts the source CRS, and one which doesnt?) I think it would be perfectly acceptable if projFunc is required to always accept a source CRS :)

I was aware of c++17, but not of 14 or 11.

The only reason is not having to change petrimaps and possibly further callers. But I guess if we need c++11, changing petrimaps is our best guess before any crazy backports.

loom is on C++11, also pfaedle. spatialjoin is on C++14.

Yupp, wrapping the projection functions in any existing code with something that also accepts a source CRS should be trivial/mechanical.

ullingerc · 2026-07-24T10:03:14Z

@yarox-1 Unfortunately due to the merge of #6 there are now conflicts. Can you check if those are resolvable in a reasonable amount of time? Otherwise please contact me and we will check on them.

…d-freiburg#6's rewrite PR ad-freiburg#5 (CRS/projection support: CRSType, getCRSType, *FromWKTProj with CRS-aware overloads, projectToCRS/CRS84/WGS84/WebMerc, lngLat<->latLng swap) predates ad-freiburg#6's large distance-computation rewrite, which touched the same WKT-parsing machinery and split the monolithic GeoTest.cpp into per-feature files. Master's (ad-freiburg#6) conventions were kept wherever the two overlapped: - CMakeLists.txt: kept ad-freiburg#6's standalone-build setup over ad-freiburg#5's temporary local scaffold. - geo/Geo.{h,tpp}: kept ad-freiburg#6's F&& forwarding-reference convention for the std::string WKT-wrapper overloads; also fixed three latent naming bugs where ad-freiburg#6's own .tpp definitions had dropped the "Proj" suffix (multiLineFromWKT/multiPolygonFromWKT/collectionFromWKT instead of ...WKTProj), leaving ad-freiburg#5's correctly-named declarations undefined. - Replaced ad-freiburg#5's `if constexpr`/`std::is_invocable_r_v`-based dispatch (used to detect whether a projFunc takes just a Point or also a CRSType) with a C++11-compatible SFINAE overload pair, since ad-freiburg#6's CMakeLists.txt targets C++11 for downstream compatibility. - Fixed a pre-existing (and now newly-exposed) declaration/definition mismatch for the std::string overload of pointFromWKTProj, latent in ad-freiburg#6 itself, now surfaced as an ambiguous-call by aligning it to the same F&& convention used everywhere else. - tests/GeoTest.cpp: restored ad-freiburg#6's minimal split-runner file; moved ad-freiburg#5's new coverage (IRI/CRS-type detection, CRS conversions, and CRS-aware WKT parsing for every geometry type) into a new tests/GeoTestCRS.cpp, following ad-freiburg#6's one-file-per-feature split. Full standalone build and test suite verified passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…Point, CRSType) Upstream (9ffed53) drops the dual-signature projFunc entirely: every WKT-parsing entry point now always passes the detected CRSType down to projFunc, and the CRS84-defaulting convenience wrappers (pointFromWKT, lineFromWKT, etc.) call projectToCRS84 explicitly themselves instead of relying on a dispatch shim. This also adds the missing CRSType-aware overloads for multiPointFromWKTProj/collectionFromWKTProj, matching the other geometry parsers. This makes the C++11 SFINAE dispatch helper (callProjFunc) added in the previous merge commit dead code, since there is no longer a single-argument projFunc signature to detect - removed it along with its two call sites in favor of upstream's direct calls. Full standalone build and test suite verified passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Crs merge pr5

ullingerc

Another round of reviews. But this looks quite nice already.

ullingerc · 2026-07-27T06:58:04Z

+  PLACEHOLDER1 = 4,
+  PLACEHOLDER2 = 5,
+  PLACEHOLDER3 = 6,
+  PLACEHOLDER4 = 7


Remove these placeholders please. They are not needed and the enum can be extended anytime in the future.

ullingerc · 2026-07-27T06:59:10Z

+template <typename T>
+Point<T> lngLatToLatLng(Point<T> lngLat) {
+  return swapCoords<T>(lngLat.getX(), lngLat.getY());
+}
+
+// _____________________________________________________________________________
+template <typename T>
+Point<T> latLngToLngLat(Point<T> latLng) {
+  return swapCoords<T>(latLng.getX(), latLng.getY());
+}
+


These are exactly identical. I understand why they are there, but we should think about it if we really want them or use swapCoords directly at the call sites.

I think there are arguments for keeping the functions and also for only using swapCoords. I would say that for some functions it is easier to follow them when having two seperate functions and not only swapping. Therefore one can always see the input CRS and output CRS. Especially when combined with for example ToWebMerc it is pretty important to know that you are in the correct CRS.
I would keep them for now, but I'm also open for just using swapCoords.

patrickbr · 2026-07-28T13:20:20Z

@yarox-1 Unfortunately due to the merge of #6 there are now conflicts. Can you check if those are resolvable in a reasonable amount of time? Otherwise please contact me and we will check on them.

Sorry for that... on the positive side, the tests compile much faster now :) (Everything else should be irrelevant for this PR).

yarox-1 · 2026-07-28T14:12:25Z

@yarox-1 Unfortunately due to the merge of #6 there are now conflicts. Can you check if those are resolvable in a reasonable amount of time? Otherwise please contact me and we will check on them.

Sorry for that... on the positive side, the tests compile much faster now :) (Everything else should be irrelevant for this PR).

No problem. I think it all works now :)

ullingerc · 2026-07-31T07:59:55Z

+  case WEB_MERCATOR:
+    return projectToWebMerc(p, baseCRS);
+  default:
+    throw std::runtime_error("Projection to unsupported CRS type.");


before we get to merging this, I would like to hear @patrickbr 's final word on this. Is throwing fine for all your call sites or do we need graceful error handling?

Throwing is fine here :)

yarox-1 added 6 commits July 9, 2026 15:27

getWKTType skips IRIs

4f6a82c

getCRSType

cbcd790

Swaps between lngLat latLng

b5f3cd6

Idea: points with crs

583b488

Idea: convertToCRS to solve projections

4c1c475

Idea: pointFrom with CRS and changed getWKT

b670983

Removed CRS Type from Point, pointFromWKT transforms to WebMerc

b8c5c0c

yarox-1 added 5 commits July 17, 2026 17:16

Added crs conversion functions with tests

2cac285

Added 'makeProjFunc' for later

8d5c435

More consistency

38fe8f6

Changed signature of projection Functions for points

64852ba

Changed projection signature for all ...FromWKT functions (auto conve…

24d001a

…rsion to CRS84 default)

ullingerc suggested changes Jul 22, 2026

View reviewed changes

patrickbr reviewed Jul 22, 2026

View reviewed changes

Code Revie Changes 1

8e4b2f3

ullingerc and others added 5 commits July 24, 2026 12:28

Projection functions now need a sourceCRS argument

9ffed53

Merge pull request #1 from ullingerc/crs-merge-pr5

139df7e

Crs merge pr5

removed unnecessary include

69a54a1

ullingerc suggested changes Jul 27, 2026

View reviewed changes

Code Review Changes

06e1ee9

yarox-1 mentioned this pull request Jul 28, 2026

Adjusted to new projection sigunature in util ad-freiburg/spatialjoin#21

Merged

yarox-1 mentioned this pull request Jul 28, 2026

Adjust geometry parsing to new projection signature in util ad-freiburg/qlever-petrimaps#47

Merged

Fixed small problem in getWKT for Collections; added more tests

7246e9a

yarox-1 marked this pull request as ready for review July 28, 2026 15:44

ullingerc reviewed Jul 31, 2026

View reviewed changes

yarox-1 added 2 commits July 31, 2026 10:58

Added invalid IRI tests and copyright

7599a17

Removed long lines from tests

5b1582f

patrickbr merged commit a3d0b2a into ad-freiburg:master Aug 4, 2026

Uh oh!

Conversation

yarox-1 commented Jul 10, 2026

Uh oh!

patrickbr commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yarox-1 commented Jul 14, 2026 • edited by patrickbr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickbr commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickbr commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ullingerc commented Jul 16, 2026

Uh oh!

ullingerc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickbr Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ullingerc commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ullingerc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickbr commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yarox-1 commented Jul 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

patrickbr commented Jul 13, 2026 •

edited

Loading

yarox-1 commented Jul 14, 2026 •

edited by patrickbr

Loading

patrickbr commented Jul 16, 2026 •

edited

Loading

patrickbr commented Jul 16, 2026 •

edited

Loading

patrickbr Jul 22, 2026 •

edited

Loading

ullingerc commented Jul 24, 2026 •

edited

Loading

patrickbr commented Jul 28, 2026 •

edited

Loading