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

EARTH_RADIUS is inconsistent #1236

Open
nhamer opened this issue Sep 17, 2017 · 9 comments · May be fixed by #1244
Open

EARTH_RADIUS is inconsistent #1236

nhamer opened this issue Sep 17, 2017 · 9 comments · May be fixed by #1244

Comments

@nhamer
Copy link

nhamer commented Sep 17, 2017

window.EARTH_RADIUS=6378137;
defines window.EARTH_RADIUS=6378137;

2bdc063a modifies Leaflet to use the same radius as S2 - 6367000

wackiness may ensue

@McBen
Copy link
Contributor

McBen commented Sep 18, 2017

value is obsolete

(only used in obsolete function getResonatorLatLng)

@nhamer
Copy link
Author

nhamer commented Sep 19, 2017

I was figuring it might get (mis)used in add-on scripts. Ought to either get changed or removed, though.

@nhamer
Copy link
Author

nhamer commented Oct 1, 2017

This is modified in Leaflet v1.0.0:
Leaflet/Leaflet#4184

@nhamer nhamer linked a pull request Oct 3, 2017 that will close this issue
@FesterCluck
Copy link
Contributor

@nhamer This value has a long history of wanting to be edited, so we should probably document the source path of the value. @McBen is right when stating it serves almost no useful purpose in core atm. This value has historically mirrored the value on the stock Intel Map, as to not lose it. The math around fixing these differences is too big for the scope of this post, except to say retaining stock Intel's opinion on the value is important, stock Intel still has the best accuracy rating on edge case links. And their opinion? 6378137.

When I first started contributing to this project I went through the same process on this constant, as others have. So I propose something different. How about we rename it to "STOCK_INTEL_EARTH_RADIUS" or something similarly self-documenting?

@McBen
Copy link
Contributor

McBen commented Oct 15, 2017

@FesterCluck I prefer clean code. Maintain (or talking) about unused/obselete code is just a wasted of time.

@nhamer
Copy link
Author

nhamer commented Oct 15, 2017

The source path of the value is Leaflet 0.7 stock, 6367000 is the correct value.

@McBen could we just remove (internal) users of it then? layer_farms_find.user.js uses its own version of the value (as does the china map offset, but that's almost certainly correct for the purpose)

@FesterCluck
Copy link
Contributor

I wrote something long, and then just realized removing it might be best. If we're worried about plugin authors, we could write a getter for the value that spits out warnings of deprecation to console for a while, then just remove it.

@johnd0e
Copy link

johnd0e commented Feb 27, 2019

@FesterCluck, @nhamer

This value has a long history of wanting to be edited, so we should probably document the source path of the value.

Could you explain it here?
What are the purpose of changing the value?
What is proper value of it?
Where is the source of S2 value? Is it really used in Ingress backend?

@FesterCluck
Copy link
Contributor

FesterCluck commented Feb 27, 2019 via email

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 a pull request may close this issue.

4 participants