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

add MercatorCoordinate meterInMercatorCoordinateUnits method #8524

Merged
merged 6 commits into from
Aug 14, 2019

Conversation

andrewharvey
Copy link
Collaborator

Launch Checklist

  • briefly describe the changes in this PR

an extension to #8516 to add a toScale method to MercatorCoordinate, which abstracts finding the scale value to convert read world units in meters to MercatorCoordinate (0 to 1).

This makes things easier for the developer, as they no longer need to do this calculation themselves when using the custom layers api.

  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page

Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

This looks great. I think it's ok to close #8516 and wait for this to go out in the next release. You've answered the original question and if we merge the docs change then we'd have to remember to remove those lines in a few weeks.

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

Thanks the pr and trying to eliminate these magic numbers from the examples!

I think it would be good if the method had a :

  • more descriptive name (scale means a lot of different things already)
  • if the we can solve the need without adding a magicish factor that is hard to explain. I think it might be better if this converted values in well defined units rather than providing a factor

Do you have any ideas on how we could make these things clearer?

Or do you think this need could be solved by adding something to LngLat instead?

@andrewharvey
Copy link
Collaborator Author

andrewharvey commented Jul 23, 2019

Good comments @ansis. I don't have all the answers or solution, I'm also open to any suggestions you or anyone else has.

more descriptive name (scale means a lot of different things already)

If we keep the method as is, then maybe transformScale()? It's the scale value for the transformation matrix to scale your real world unit data (in meters) down into the MercatorCoordinate projection which is 0 to 1.

if the we can solve the need without adding a magicish factor that is hard to explain. I think it might be better if this converted values in well defined units rather than providing a factor

Okay, so if we instead provided a function like meters2unitMercator(distance, lat) which converted meters into unit mercator, then users could get the transformation scale as distance / meters2unitMercator(distance, lat). That feels a bit more complex to me though...

Do you have any ideas on how we could make these things clearer?

🤔 I wish I did!:

Or do you think this need could be solved by adding something to LngLat instead?

Since this method is related to the coordinate system used by MercatorCoordinate (0 to 1 or web mercator), I think it's best to have it here. Placing it over with LngLat I think would lead to more confusion.

@ansis
Copy link
Contributor

ansis commented Jul 25, 2019

@andrewharvey hmm, throwing out some more ideas:

metersToMercatorUnits(meters: number) {
    return meters / circumferenceAtEquator * mercatorScale(latFromMercatorY(this.y));
}
scale: modelAsMercatorCoordinate.metersToMercator(1)

or

just keeping what you have already but renaming it to meterInMercatorUnits()

or

class LngLat {
    ...
    lengthOfParallel() {
        return circumferenceAtEquator * mathgoeshere(this.lat);
    }
}

...

var modelTransform = {
    ...
    scale: 1 / modelOriginLngLat.lengthOfParallel()
}

@andrewharvey
Copy link
Collaborator Author

Good suggestions @ansis .

I'm not against putting this method on LngLat, it's just that LngLat currently doesn't concern itself with projection, it just represents a simple geographic (unprojected) coordinate. This scale factor is calculated on the Mercator projection, and being use to help with the special MercatorCoordinate coordinate reference system used by the custom layers api, so for that reason I'd favour, either option 1 or 2.

I think meterInMercatorCoordinateUnits() is better, since Mercator itself is just a projection, not a full coordinate reference system, using the term MercatorCoordinate makes it clear we're talking about the MercatorCoordinate coordinate reference system used by the custom layers api (ie. the one where values ranges from 0 to 1, instead of Web Mercator which ranges from roughly -20,000,000 to 20,000,000).

meterInMercatorCoordinateUnits makes it clear we're simply converting the units for a distance measurement of 1 meter into MercatorCoordinate's units.

Then the answer to what is a meterInMercatorCoordinateUnits quite simply becomes the scale to convert data in meters to MercatorCoordinate, ie.

scale: modelAsMercatorCoordinate.meterInMercatorCoordinateUnits()

@andrewharvey
Copy link
Collaborator Author

I've just updated the PR with this, let me know what you think.

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

Looks good!

@ansis
Copy link
Contributor

ansis commented Aug 13, 2019

All the examples have been moved to https://github.com/mapbox/mapbox-gl-js-docs so those changes need to be moved to a separate pr. This should be good to merge then!

@andrewharvey andrewharvey changed the title add MercatorCoordinate toScale method add MercatorCoordinate meterInMercatorCoordinateUnits method Aug 14, 2019
@andrewharvey
Copy link
Collaborator Author

Thanks Ansis, I've dropped out the example from here and will open a PR over at the docs repo for the example update..

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