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

Refactor fill extrusion texture/fbo cache #5328

Merged
merged 1 commit into from
Sep 20, 2017
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Sep 19, 2017

@lbud @jfirebaugh this is just a quick exploration of how caching textures in style layer objects would look. I would highly benefit from something like this in #5253 because I need to cache two types of textures that differ from the fill extrusion use case — a half-float screen-sized texture downscaled by 4, and a 256x1 RGBA color ramp texture.

Heatmap-specific caching logic in painter.js would look really bad, and while I agree it's awkward to cache render-specific stuff in style layers and look forward to RenderLayer-introducing refactor, for now this seems like a lesser evil than writing lots of layer-specific code in painter.js.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality (covered by render tests)
  • document any changes to public APIs
  • post benchmark scores there are no benchmarks with extrusions
  • manually test the debug page

@mourner mourner mentioned this pull request Sep 20, 2017
36 tasks
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

BTW, do these GL resources get deleted if the layer or map is removed? (This may have already been an issue with the existing code.)

@mourner mourner merged commit fbf6145 into master Sep 20, 2017
@mourner mourner deleted the refactor-extrusion-cache branch September 20, 2017 19:39
@mourner
Copy link
Member Author

mourner commented Sep 20, 2017

@jfirebaugh thanks! I think this is an issue with existing code independent of this PR. Not sure how WebGL handles those resources though — does a WebGL texture/framebuffer get destroyed automatically if it's not bound to a context and reached zero references for GC?

@jfirebaugh
Copy link
Contributor

Yeah, the GL resource will be released when the JS object is GC'd, but generally its' a good idea to manually release them when you know they're unused. GC doesn't always happen as quickly as you'd like, or sometimes there can be an outstanding reference to a GL object that's actually no longer needed.

@mourner
Copy link
Member Author

mourner commented Sep 20, 2017

@jfirebaugh I think we can add another noop method to style layers, destroy, that has clean up code in child classes if necessary. We can later move them after the RenderLayer refactor.

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