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.

Sign up for GitHub

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

Jump to bottom

clean up matrix creation #3790

Merged
merged 4 commits into from
Dec 14, 2016
Merged

clean up matrix creation #3790

merged 4 commits into from
Dec 14, 2016

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Dec 13, 2016 •
edited
Loading

This cleans up a bunch of matrix and antialiasing code.

  • removes the confusing altitude and replaces it with fov
  • fixes text blurriness at non-default FOVs
  • removes the hack added in Fix broken map on pitch=0 on some systems #3740 and adds a different fix
  • makes pitched line antialiasing clearer and removes lineStretch and lineAntialiasingMatrix

related prs:
mapbox/mapbox-gl-shaders#41
mapbox/mapbox-gl-test-suite#193

benchmark master 32fcbfe matrix-cleanup d2f5d09
map-load 207 ms 152 ms
style-load 95 ms 94 ms
buffer 1,027 ms 1,001 ms
fps 60 fps 60 fps
frame-duration 4.5 ms, 0% > 16ms 4.6 ms, 0% > 16ms
query-point 1.00 ms 1.14 ms
query-box 85.19 ms 79.48 ms
geojson-setdata-small 7 ms 5 ms
geojson-setdata-large 299 ms 286 ms

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

TODO:

  • debug the render test that's failing on ci but not locally
  • open issue about porting to -native

@lucaswoj @mourner

mourner
mourner approved these changes Dec 13, 2016
View reviewed changes
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Wonderful!

@mourner
Copy link
Member

mourner commented Dec 13, 2016

line-offset property-function render test is failing though.

@ansis ansis mentioned this pull request Dec 13, 2016
Open
@mourner
Copy link
Member

mourner commented Dec 14, 2016

Good to merge, pending a package.json conflict resolution.

@mourner
Copy link
Member

mourner commented Dec 14, 2016

Needs rebase and moving shader changes here after mapbox-gl-shaders was moved to the repo.

ansis added 4 commits December 14, 2016 11:07
@ansis
fix text blurriness at different FOVs
c52a096 
This replaces a hardcoded shader approximation with a more correct,
FOV-dependent one.

`gl_Position.w - 0.5`
is replaced with
`gl_Position.w / tan(fov)`

The `/ tan(fov)` is handled by multiplying `1 / tan(fov)` into `u_gamma`
outside the shader.

I think `- 0.5` was just chosen as something that looked right for the
default fov.
@ansis
replace altitude with fov
eb6c659 
`altitude` was a terribly-named variable that was used to indirectly
control the fov. This should eliminate some confusion.

`altitude` was equivalent to `cameraToCenterDistance / height`
@ansis
fix matrix z range and remove hack
0b5520f 
This should fix the issue behind #2270 and remove the need for the hack
added in #3740.
@ansis
clearer pitched line antialiasing
ef5582d 
Project the extrusion and compare it's projected pixel length with the
actual pixel length and adjust antialiasing accordingly.

The previous approach calculated the adjustment much more indirectly and
had no intuitive explanation.
@ansis ansis mentioned this pull request Dec 14, 2016
Closed
@ansis ansis merged commit ef5582d into master Dec 14, 2016
@ansis ansis deleted the matrix-cleanup branch December 14, 2016 19:22
This was referenced Dec 14, 2016
Closed
Merged
@jfirebaugh jfirebaugh mentioned this pull request Dec 21, 2016
Closed
@mourner mourner mentioned this pull request Nov 21, 2018
Closed
@fifiDesPlaines fifiDesPlaines mentioned this pull request May 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mourner mourner mourner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ansis @mourner