locationtech / proj4j

Java port of the Proj.4 library for coordinate reprojection
Other
184 stars 73 forks source link

Make Registry remember projection descriptions #30

Closed noberasco closed 5 years ago

noberasco commented 5 years ago

Method initialize() in class Registry maintains human-readable descriptions for the various projections. However, these descriptions are not exported. Actually, register() method completely ignores them, so they are lost.

This pull request introduces a small refactoring to make Registry remember these descriptions in such a way that, after calling getProjection() to retrieve a projection, it is possible to call getDescription() on the returned projection to get this human-readable description.

echeipesh commented 5 years ago

Not having the historical perspective here I'm a little confused why the description is provided to the Registry at all and also I'm unsure why it is useful to propagate this information.

Spot checking it seems that each Projection subclass already provides a toString implementation that closely reflects its description:

https://github.com/locationtech/proj4j/blob/bcc1317efda25125d74fb10951d7b18131c288c0/src/main/java/org/locationtech/proj4j/Registry.java#L211

https://github.com/locationtech/proj4j/blob/bcc1317efda25125d74fb10951d7b18131c288c0/src/main/java/org/locationtech/proj4j/proj/LambertAzimuthalEqualAreaProjection.java#L309-L311

https://github.com/locationtech/proj4j/blob/bcc1317efda25125d74fb10951d7b18131c288c0/src/main/java/org/locationtech/proj4j/Registry.java#L225

https://github.com/locationtech/proj4j/blob/bcc1317efda25125d74fb10951d7b18131c288c0/src/main/java/org/locationtech/proj4j/proj/McBrydeThomasFlatPolarSine2Projection.java#L67-L69

this seems pretty consistent.

@noberasco could you clarify what problem this PR solves for you?

noberasco commented 5 years ago

If you simply want to pick a projection and work with it, none of this is necessary. Instead, my application allows users to custom-configure coordinate systems and lets them select, amongst other things, the desired projection. In this use case, I need a complete listing of available projections, and a human-readable name (aka our 'description') for each projection.

Now, let's go to the issue this PR attempts to address...

Registry (which maintains the 'complete listing of available projections' I mentioned in my use case scenario) configures multiple projections that point to the same subclass of Projection.

Example: register("latlong", LongLatProjection.class, "Lat/Long (Geodetic alias)"); register("longlat", LongLatProjection.class, "Lat/Long (Geodetic alias)"); register("latlon", LongLatProjection.class, "Lat/Long (Geodetic alias)"); register("lonlat", LongLatProjection.class, "Lat/Long (Geodetic)");

These are simply aliases of the same projection. These cannot be merged into one, as all name variants are actively in use in proj.4 init strings. However, it is true that their descriptive strings could safely be identical.

A better example: register("tmerc", TransverseMercatorProjection.class, "Transverse Mercator"); register("utm", TransverseMercatorProjection.class, "Universal Transverse Mercator (UTM)");

These are different projections, which happen to be implemented by the same class. To be able to distinguish between them, you could either:

The separate class approach is already used with WinkelTripelProjection (which is a subclass of AitoffProjection and does nothing more than altering its description and setting a single parameter).

Both approaches here are feasible. I picked the 'expose the descriptions' one simply because Registry is already doing the same thing for projection names (see method getProjection: projection names are supplied externally to instances of Projection subclesses), so I applied the same logic to projection descriptions.

echeipesh commented 5 years ago

@noberasco Thank you for the clarification, sorry for slow response cycle.

TransverseMercatorProjection does look like it's overloaded for convenience sake. If that is only sticking point and suggestion is to break it out into two classes where UniversalTransverseMercatorProjection extends TransverseMercatorProjection I think it is a perfectly legitimate change and I'd be happy to "+1" that.

However, I want to resist the urge to add more dynamic fields to Projection especially if it seems like it may be related to application logic. I don't know if this actually happens in reality, but it's certainly possible for instances of Projection class to be created outside of those contained in the Registry. Since the projection description is really added in the registry initialization would it be just as acceptable to add getProjectionDescription method on that class and use it to get at this information?

Apologies I'm being difficult, I'm open to other options or more forceful argument why description on Projection is a good move in general.

echeipesh commented 5 years ago

Closing this PR because I don't think it can be merged as is. I aim to be good FOSS citizen so I'm open to discussing the issue further and try to solve underlying problem.