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

GeoAPI wrappers for PROJ4J #115

Merged
merged 7 commits into from
Mar 22, 2025
Merged

GeoAPI wrappers for PROJ4J #115

merged 7 commits into from
Mar 22, 2025

Conversation

desruisseaux
Copy link
Contributor

This is a proposal to add a module for viewing PROJ4J as an implementation of the GeoAPI 3.0 interfaces. GeoAPI is an OGC standard providing Java interfaces derived from ISO 19111 / OGC Topic 2 — Referencing by coordinates. By using this optional module, developers could use PROJ4J in a way closer to the ISO 19111 conceptual model. It also gives to developers some implementation independence, making easier for them to use different GeoAPI implementations depending on their needs. For example, they could use PROJ4J for common two-dimensional cases, and switch to another implementation when they need four-dimensional cases, or engineering CRS, etc. Other GeoAPI implementations include Apache SIS, GeoTools and PROJ. The existence of GeoAPI wrappers also make possible to run GIGS tests against PROJ4J, or to format CRS in JSON with the CRS JSON prototype (for discussion only, not an approved standard).

The two first commits of this pull request rearrange the pom.xml files for avoiding redundancies. Those two commits may be useful regardless if the rest of this pull request is approved or not. The other commits create a new module named proj4j-geoapi. The wrappers are provided in that separated module. Users of the main proj4j module are unaffected and can continue to ignore GeoAPI if they wish.

This pull request is initially in draft mode and is created for collecting comments. The purpose is to see if the principle of creating a new module in the PROJ4J project is accepted. If not, those wrappers would need to be provided in an independent project.

@desruisseaux desruisseaux marked this pull request as draft March 10, 2025 09:30
@pomadchin
Copy link
Member

Hi @desruisseaux I think it's a great addition! Thanks for doing it. I'll take a closer look a bit later, but so far looks very good.

I also updated the CI so it's not complaining about the action steps versions. Another minor thing is a complaining eclipse bot - it wants the email that's been used to sign ECA.

@desruisseaux
Copy link
Contributor Author

Thanks @pomadchin ! Can you point me to the place where I can sign the Eclipse ECA?

@pomadchin
Copy link
Member

pomadchin commented Mar 10, 2025

@desruisseaux sure; signing https://www.eclipse.org/legal/ECA.php will make validator happy! 🤞

There is a button to sign online, it will prompt you to log in / create a new eclipse account (I don't remember the exact flow unfortunately).

image

…d in modules.

The intend is to avoid more duplication with the introduction of additional modules.
The result should be identical, except the following:
In the `maven-deploy-plugin`, the configuration was:

* <deployAtEnd>true</deployAtEnd> in the `proj4j` module.
* <updateReleaseInfo>true</updateReleaseInfo> in the `epsg` module.

With this commit, the configuration become the following for the two modules:

    <deployAtEnd>true</deployAtEnd>
    <updateReleaseInfo>true</updateReleaseInfo>

Everything else should be effectively identical.
in order to manage the dependency version in a single place for all modules.
Test the transform of a point from geographic to projected CRS.
A side-effect of this work is the addition of `proj4j(…)` methods
as the reverse of `geoapi(…)` methods.
@desruisseaux desruisseaux marked this pull request as ready for review March 11, 2025 16:12
@desruisseaux
Copy link
Contributor Author

The pull request is completed and ready for review. It has some tests, in particular in the Proj4J → GeoAPI direction. Tests of the opposite direction (GeoAPI → Proj4J) are missing, but may be added progressively in the future.

@pomadchin
Copy link
Member

pomadchin commented Mar 11, 2025

Thanks @desruisseaux, great work 🎉 I'll take a closer look later this week. We are 100% going to merge it though 🔥 there are no reasons to not do so.

@desruisseaux
Copy link
Contributor Author

Thanks! I just saw that the build is failing because a dependency used for the test requires Java 11. This is only for the tests, not the main code. Possible fix can be either use Java 11 for building the project but still target Java 8 with the --release 8 compiler option, or I try to find a Java 8 compatible dependency for the test (I think that it is possible).

@pomadchin
Copy link
Member

pomadchin commented Mar 11, 2025

Will leave the decision on it up to you, both options sound good. Also using JDK11 for builds with the JDK8 targets is probably a step we need to do regardless eventually.

Java version has been verified with `javap`.
@desruisseaux
Copy link
Contributor Author

I have set the Java version to 21 for the build in order to use a SDK that is still supported. I have verified that Java 8 byte codes were generated with the following command:

javap --class-path core/target/proj4j-1.3.1-SNAPSHOT.jar \
    -verbose org.locationtech.proj4j.proj.Projection | more

Relevant output snippet (52 stands for Java 8):

  minor version: 0
  major version: 52

@pomadchin pomadchin self-requested a review March 17, 2025 00:01
Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is massive, thank you! Also gz with the first contirbution!

I'll try to cut a release shortly.

@pomadchin pomadchin merged commit 9016292 into locationtech:master Mar 22, 2025
2 checks passed
*
* @author Martin Desruisseaux (Geomatys)
*/
public class ServicesTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd probably need more tests, but it's fine for now as a first pass.

@desruisseaux
Copy link
Contributor Author

Thanks! I just sent a short announcement on the Apache Geospatial mailing list.

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.

2 participants