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

MERCATOR_A constant unused #158

Closed
lights0123 opened this issue Jan 2, 2021 · 3 comments
Closed

MERCATOR_A constant unused #158

lights0123 opened this issue Jan 2, 2021 · 3 comments
Assignees
Labels
📝 documentation Improvements or additions to documentation ❓ question Further information is requested
Milestone

Comments

@lights0123
Copy link

in src/utils/constants.js, MERCATOR_A is defined as a file-local constant:

const MERCATOR_A = 6378137.0; // 900913 projection property

However, when exported, the very similar—but not quite the same—EARTH_RADIUS constant is used instead:

MERCATOR_A: EARTH_RADIUS, // 900913 projection property

That makes the earlier defined MERCATOR_A constant unused. Is this a copy/paste error, or is this truly the way it should be?

@jscastro76 jscastro76 added the ❓ question Further information is requested label Jan 2, 2021
@jscastro76
Copy link
Owner

jscastro76 commented Jan 2, 2021

Hi @lights0123
That's a great question, the fact is that the most precise value is const MERCATOR_A = 6378137.0; // 900913 projection property, is the one that I would use, but is not the one used by Mapbox which includes hardcoded the following:

/*
* Approximate radius of the earth in meters.
* Uses the WGS-84 approximation. The radius at the equator is ~6378137 and at the poles is ~6356752. https://en.wikipedia.org/wiki/World_Geodetic_System#WGS84
* 6371008.8 is one published "average radius" see https://en.wikipedia.org/wiki/Earth_radius#Mean_radius, or ftp://athena.fsv.cvut.cz/ZFG/grs80-Moritz.pdf p.4
*/
export const earthRadius = 6371008.8;

So, to be accurate with Mapbox calculations I decided to modify it to the value they use.

Are you finding any issue in your calculations?

@lights0123
Copy link
Author

Nope. However, I'm wondering if the unused constant should be removed and a comment added explaining the use of EARTH_RADIUS.

@jscastro76 jscastro76 self-assigned this Jan 2, 2021
@jscastro76 jscastro76 added the 📝 documentation Improvements or additions to documentation label Jan 2, 2021
@jscastro76 jscastro76 added this to the v.2.1.6. milestone Jan 2, 2021
jscastro76 added a commit that referenced this issue Jan 5, 2021
Minor version by [@jscastro76](https://github.com/jscastro76), some enhancements and bugs.

#### ✨ Enhancements

- #111 Show the dimensions of a model
- #151 Remove auxiliary test method on `CameraSync` used to debug #145
- #156 Create an Orthographic view mode
- #159 Create an example for FOV and Orthographic.
  - Added to [09-raycaster.html](https://github.com/jscastro76/threebox/blob/master/examples/09-raycaster.html) as it impacts in raycast and shows fill-extrusions and 3D models together
- #161 Remove obsolete code that is avoiding to be used from React

#### 🪲 Bug fixes
- #152 `obj.raycasted` is ignored when an object is hidden and again visible.
- #157 Bug draggging after removing an object.
- #160 Bug using `utils.equal`

#### 📝 Documentation
- #158 MERCATOR_A constant unused, added an code comment to explain why it was deprecated (nor removed)
- Updated [documentation](/docs/Threebox.md) (`tb.orthographic`, `tb.fov`)
- Updated [README.md](/).
- Updated [Examples](/examples) documentation.
@jscastro76
Copy link
Owner

@lights0123 updated the comment, but I decided not to remove the local variable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📝 documentation Improvements or additions to documentation ❓ question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants