-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Resolve Z units to meters #3509
Conversation
… future other 3D types)
@@ -453,6 +453,10 @@ class Transform { | |||
mat4.rotateX(m, m, this._pitch); | |||
mat4.rotateZ(m, m, this.angle); | |||
mat4.translate(m, m, [-this.x, -this.y, 0]); | |||
mat4.scale(m, m, [1, 1, | |||
// scale vertically to meters per pixel (inverse of ground resolution) | |||
(Math.pow(2, this.zoom) * 512) / (2 * Math.PI * 6378137 * Math.abs(Math.cos(this.center.lat * (Math.PI / 180)))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document or split this up a bit more? maybe replace 2 * Math.PI * 6378137
with a circumferenceOfEarth
constant
This looks good. Have you checked this visually? If you make a cube from real-world distances does it get rendered as a cube?
yeah, I think so |
@ansis yep |
* Resolve Z units to meters (and move z scaling to transform for use in future other 3D types) Fixes #3385.
* Resolve Z units to meters (and move z scaling to transform for use in future other 3D types) Fixes #3385.
* Resolve Z units to meters (and move z scaling to transform for use in future other 3D types) Fixes #3385.
* Eliminate fill-extrude-{height,base} in favor of separate fill-extrusion type * Resolve Z units to meters (#3509)
Fixes #3385.
I branched off the fill extrusion separate type branch and would like to merge this back into there. Lint tests are passing because of the new ESLint formatting — @mourner fixed this on master. I'd like to merge this (not passing) into the fill-extrusion-type branch and then just rebase once there.
My question is this: this line is affected when the map bearing is non-zero. Then here I'm introducing a z scaling in the same matrix. Effectively this means there is a single test in map.test.js failing here, as
— before
toFixed
that number is something like -2-13, so it introduces a miniscule change. That's the only test that fails, and all non-extrusion render tests pass; is it okay to just update this test to parse/round a bit differently to make this test pass? Or should the matrix scaling happen elsewhere/will there be unintended consequences of scaling here (after therotateZ
)?@ansis @jfirebaugh @lucaswoj