Skip to content

Conversation

@jdroenner
Copy link
Member

  • I added an entry to CHANGELOG.md if knowledge of this change could be valuable to users.

Here is a brief summary of what I did:

clap = { version = "4.5", features = ["derive"] }
clap_derive = "4.5"
config = "0.15"
crs-definitions = { git = "https://github.com/jdroenner/crs-definitions.git", branch = "area_of_use_and_extent"}
Copy link
Contributor

Choose a reason for hiding this comment

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

würdest du das so machen wollen, also die definitionen ins binary legen?

das ding was du geforkt hast hat keine Lizenz, können wir so nicht benutzen oder?

Copy link
Member Author

Choose a reason for hiding this comment

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

die Alternative wäre ja die irgendwie in eine Map zu legen. Das sind aber ja eigentlich Konstanten...

Copy link
Member Author

Choose a reason for hiding this comment

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

im Zweifel können wir das auch nachbauen oder eben anpassen. Das ist ja nichts magisches. Aber vielleicht fragen wir einfach nach ner Lizenz. Die Entries kann man alle mit Proj generieren.

util::{crs_definitions::StaticAreaofUseProvider, proj_projector::ProjAreaOfUseProvider},
};

pub enum MixedCoordinateProjector {
Copy link
Contributor

Choose a reason for hiding this comment

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

confusing name. why not a common trait?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a common trait. This enum simply mixes Geodesy and Proj in one type since there are cases where we can not create a Proj Projector and need to fallback.

def: Def,
}

impl AreaOfUseProvider for StaticAreaofUseProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not implement the trait on Def?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to keep external types wrapped so we do not need to use them everywhere. It was hard enough to find and change all places where Proj was used.

impl Transformer<geodesy::ctx::Minimal> {
pub fn pro4_string_modifications(code: u16, proj4: &str) -> String {
match (code, proj4) {
(3857, _) => proj4.replace("merc", "webmerc"),
Copy link
Contributor

Choose a reason for hiding this comment

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

guess this should be documented

Copy link
Contributor

Choose a reason for hiding this comment

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

also: are there any other cases that require this?

would it make sense to curate this in the crs definitions crate?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. This is a very complex topic. I do not know if other projections need this. Also the crs_definitions crate is not bound to Geodesy and as i see it this is something only geodesy requires. We should upstream this to the geo-geodesy crate but there are other things that needs to be done there first.

});
}

if source.code() > u32::from(u16::MAX) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isnt GOOGLE crs larger than u16?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes but it is not a real EPSG code. This would need something handling "alias" names

}
}

impl<C: geodesy::ctx::Context> Transformer<C> {
Copy link
Contributor

@michaelmattig michaelmattig Jan 19, 2026

Choose a reason for hiding this comment

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

are there even multiple geodesy context implementations?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes there are two and they plan a "ParallelContext" upstream

};

pub enum MixedCoordinateProjector {
ProjProjector(super::proj_projector::ProjCoordinateProjector),
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this line: in general we need to think about whether the approach to use geodesy because of performance and fall back to proj only when geodesy does not support a projection suits all our use cases. can we be reasonably certain that we will not need some crs in the future that geodesy does not support and where we need good performance? because then we would need to implement proj context reusage anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

well this is a very interesting question. One "projection" not implemented in Godesy is the GEOS projection. However, we could create a PR if we need it (it is not very complex). Basically all non-EPSG projections are not supported but this is not really Geodesys fault. Geodesy should add a WKT parser... Currently it requires the proj4 strings and we get them from crs-definitions and there are only EPSG codes listed.

Copy link
Member Author

Choose a reason for hiding this comment

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

currently, all projections we use should work. However, if someone uses DHDN then we have a problem.

pub struct Transformer<C: geodesy::ctx::Context = geodesy::ctx::Minimal> {
ctx: C,
source_crs: u16,
source: geodesy::ctx::OpHandle,
Copy link
Contributor

Choose a reason for hiding this comment

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

why store the ophandle?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is what you use to call the "fwd" and "rew" projection

ctx: C,
source_crs: u16,
source: geodesy::ctx::OpHandle,
source_latlon: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this not derivable fro the source/target crs?

Copy link
Member Author

Choose a reason for hiding this comment

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

well i derive it by looking for "latlong" in the proj4 string. I do not think there is a better way at the moment. Since we use this for every coordinate we should remember this instead of the proj string. However, if you see a better way please let me know :)

#[derive(Debug, Clone)]
pub struct Transformer<C: geodesy::ctx::Context = geodesy::ctx::Minimal> {
ctx: C,
source_crs: u16,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this always epsg code? are all relevant projections describable by this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Geodesy currently parses the proj4 strings to create objects. We need to get the proj4 strings somewhere and the easiest way is to get them from the crs-definitions crate (this is what geo-geodesy does). The crs-definitions crate only has EPSG codes at the moment. We would need to add more e.g. ESRI if we need them

let source_latlon = source_def.proj4.starts_with("+proj=longlat");
let target_latlon = target_def.proj4.starts_with("+proj=longlat");

let source_proj_string = Self::pro4_string_modifications(source_crs, source_def.proj4);
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we store the definitions "correctly" in the first place instead of modifying them each time?

Copy link
Member Author

Choose a reason for hiding this comment

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

of the crs-definitions should be useable by other crates then no. If we want them only for geodesy then yes.


pub fn transform_coord(&self, coord: Coordinate2D) -> Result<Coordinate2D, Error> {
let mut geodesy_coord = if self.source_latlon {
[geodesy::coord::Coor2D::gis(coord.x, coord.y)]
Copy link
Contributor

Choose a reason for hiding this comment

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

so we need to copy all data. does proj do this too?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think we can also implement a trait. We need to look that up.

[geodesy::coord::Coor2D::raw(coord.x, coord.y)]
};
self.ctx
.apply(self.source, geodesy::Direction::Inv, &mut geodesy_coord)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

why does geodesy look up the op in the context each time? can we prevent this? we always use the same op but it does a registry lookup each time

Copy link
Member Author

Choose a reason for hiding this comment

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

it is a very strange design. We would need to look into this.

*/
}

pub fn geodesy_ctx() -> geodesy::ctx::Minimal {
Copy link
Contributor

Choose a reason for hiding this comment

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

thie Minimal context is also not Send or Sync or is it?

Copy link
Member Author

@jdroenner jdroenner Jan 21, 2026

Choose a reason for hiding this comment

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

nope but it is fast. There is an issue for a ParallelContext. Maybe this would be Send or Sync.

};

pub struct ProjCoordinateProjector {
pub from: SpatialReference,
Copy link
Contributor

Choose a reason for hiding this comment

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

can this not be derived from the Proj instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

not in a way that would be halpfull

) -> Result<SpatialGridDescriptor> {
let proj_from_to =
CoordinateProjector::from_known_srs(in_srs, params.target_spatial_reference)?;
MixedCoordinateProjector::from_known_srs(in_srs, params.target_spatial_reference)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't it still make sense to try to reuse the projector? did you measure the overhead of instantiation?

Copy link
Member Author

Choose a reason for hiding this comment

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

i created a small benchmark. It is much faster then proj.. You can add more benches if you have ideas how to measure that.

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.

3 participants