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

Adds EPSG:7855 to known projections. #6857

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Adds EPSG:7855 to known projections. #6857

merged 2 commits into from
Aug 31, 2023

Conversation

na9da
Copy link
Collaborator

@na9da na9da commented Aug 31, 2023

What this PR does

Adds EPSG:7855 to known projections.
This should fix ideal zoom for some layers.

Test me

Check ideal zoom before and after

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

Copy link
Contributor

@nf-s nf-s left a comment

Choose a reason for hiding this comment

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

LGTM!

I can't remember much about this stuff, but is there a reason why some use of Reproject uses the terriajs Proj4Definitions instead of the terriajs-server proj4lookup service?

For example - in GeoJsonMixin the CRS needs to be in the proj4lookup service in order to reproject features

const needsReprojection = proj4ServiceBaseUrl
? await Reproject.checkProjection(proj4ServiceBaseUrl, code)
: false;
if (needsReprojection) {
try {
filterValue(geoJson, "coordinates", function (obj, prop) {
obj[prop] = filterArray(obj[prop], function (pts) {
if (pts.length === 0) return [];
return reprojectPointList(pts, code);
});
});
return geoJson;
} catch (e) {
throw TerriaError.from(e, "Failed to reproject geoJSON");
}
} else {
throw new DeveloperError(
"The crs code for this datasource is unsupported."
);
}
}

It might be worth adding EPSG:7855 to terriajs-server too

Also, this ties into the general issues with GDA2020 and GDA94 that we haven't resolved - in that the transformation from GDA2020 to WGS84, and GDA94 to WGS84 is null - but transformation from GDA94 to GDA2020 is not null.

But that is definitely for another day 🙂

@na9da
Copy link
Collaborator Author

na9da commented Aug 31, 2023

Good observations @nf-s.

I think the Proj4Definitions is a carry over from v7. It is possible that the Reproject module was added later. We could look at switching to Reproject everywhere in code. In this case though, it might involve an async step to call Reproject.checkProjection so that a projection not available locally gets imported from the server.

Also I think, the Reproject API could be rewritten as reproject and reprojectAsync or something to avoid a checkProjection step. We could even split the projection code into a new package shared between the server and client. I feel it is best done as part of some other bigger refactoring work.

@na9da
Copy link
Collaborator Author

na9da commented Aug 31, 2023

@nf-s terriajs-server change is here - TerriaJS/terriajs-server#150

@na9da na9da merged commit f1cc09b into main Aug 31, 2023
3 checks passed
@na9da na9da deleted the add-proj-7855 branch August 31, 2023 06:00
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