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

use anisotropic filtering for raster tiles #1064 #4443

Merged
merged 1 commit into from
Mar 21, 2017

Conversation

andrewharvey
Copy link
Collaborator

@andrewharvey andrewharvey commented Mar 17, 2017

Launch Checklist

  • briefly describe the changes in this PR
    Implements anisotropic filtering for raster tiles Use anisotropic filtering for raster tiles in perspective view #1064

  • write tests for all new functionality
    Rendering test fixtures may need to be updated, apart from that I don't think it's something that we can/need unit tests for.

  • document any changes to public APIs
    N/A

  • post benchmark scores
    I ran a modified fps benchmark with satellite-streets at pitch 60 and bearing 45. Got around the same 60FPS on both.

  • manually test the debug page
    Before/After
    selection_489
    selection_490

  • test it falls back on unsupported devices
    Not sure how to do this.

  • investigate if it makes any difference that this is enabled even when the map isn't pitched
    My fps benchmark didn't seem to indicate any performance issues, so I think it's fine as is.

  • "The filtering level used should depend on the current pitch."
    Do we need to do this, or can we just use the max level?

@andrewharvey andrewharvey changed the title use anisotropic filtering for raster tiles #1064 [wip] use anisotropic filtering for raster tiles #1064 Mar 17, 2017
@andrewharvey andrewharvey changed the title [wip] use anisotropic filtering for raster tiles #1064 use anisotropic filtering for raster tiles #1064 Mar 17, 2017
const max = gl.getParameter(ext.MAX_TEXTURE_MAX_ANISOTROPY_EXT);
gl.texParameterf(gl.TEXTURE_2D, ext.TEXTURE_MAX_ANISOTROPY_EXT, max);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Are any of these values cachable for the lifetime of the page? Reducing the # of per-frame calls to WebGL APIs is important for performance. You may want to wrap that caching behaviour in the painter module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, good point!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@andrewharvey andrewharvey force-pushed the 1064-anisotropic-filtering branch from eb119a0 to 8f139a8 Compare March 19, 2017 04:02
@mourner
Copy link
Member

mourner commented Mar 20, 2017

Awesome! Do you know if it has a noticeable impact on performance? E.g. should we just have it enabled all the time, or add an option to turn it off?

@andrewharvey
Copy link
Collaborator Author

Awesome! Do you know if it has a noticeable impact on performance? E.g. should we just have it enabled all the time, or add an option to turn it off?

I ran a modified fps benchmark with satellite-streets at pitch 60 and bearing 45. Got around the same 60FPS on both.

I also did manual testing, rotating and pitching the map with the mouse and couldn't notice any performance degradation. So I think it's simpler to leave it on.

@mourner
Copy link
Member

mourner commented Mar 20, 2017

@andrewharvey fps benchmark is not very useful once it reaches 60fps — I think we should remove it. You might get more out of the frame-duration benchmark.

@andrewharvey
Copy link
Collaborator Author

streets style no impact as expected

benchmark master c189bbf 1064-anisotropic-filtering 8f139a8
frame-duration 6.1 ms, 0% > 16ms 6.2 ms, 0% > 16ms

satellite style with bearing: 0, pitch: 0 (default top down)

benchmark master c189bbf 1064-anisotropic-filtering 8f139a8
frame-duration 6.1 ms, 0% > 16ms 1.3 ms, 0% > 16ms

satellite-streets style with bearing: 0, pitch: 0 (default top down)

benchmark master c189bbf 1064-anisotropic-filtering 8f139a8
frame-duration 6.1 ms, 0% > 16ms 5.1 ms, 0% > 16ms

satellite style with bearing: 45, pitch: 60 (most extreme view)

benchmark master c189bbf 1064-anisotropic-filtering 8f139a8
frame-duration 6.2 ms, 0% > 16ms 3.1 ms, 0% > 16ms

satellite-streets style with bearing: 45, pitch: 60 (most extreme view)

benchmark master c189bbf 1064-anisotropic-filtering 8f139a8
frame-duration 5.9 ms, 0% > 16ms 9.9 ms, 5% > 16ms

@mourner
Copy link
Member

mourner commented Mar 21, 2017

Huh, so it's actually faster with anisotropic filtering most of the time? 🎉

@andrewharvey
Copy link
Collaborator Author

Huh, so it's actually faster with anisotropic filtering most of the time?

Yeah, unless there is an issue with the benchmark. I ran a second time to be sure and results were the same.

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