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

Heatmaps layer #5253

Merged
merged 34 commits into from
Oct 6, 2017
Merged

Heatmaps layer #5253

merged 34 commits into from
Oct 6, 2017

Conversation

mourner
Copy link
Member

@mourner mourner commented Sep 7, 2017

Closes #4756. Still a work in progress, but pretty close.

Technical tasks

  • rebase on top of master with expressions
  • refactor to take advantage of the new RenderTexture class (edit: not applicable for now)
  • make sure viewportTexture caching works properly and is aligned with master
  • possibly increase the number of values in the color ramp lookup array (update: no need, very small difference)
  • add comments for tricky heatmap fragment shader math
  • fix halos around points
  • commit a small GeoJSON fixture for the heatmaps.html debug page
  • handle the need to have the first color stop with zero alpha value
  • settle sensibly on all default property values
  • fix issues when resizing the map
  • cache color ramp GL texture object
  • make sure all texture preparations happen in the upload step rather than in the middle (edit: nothing to do in the upload step)
  • take advantage of the new tile mask feature to get rid of occasional flickering while zooming (used a simpler hack)
  • don't create viewport buffer/vao on every frame
  • investigate prerendering a reusable kernel texture for performance no need for now, heatmap rendering is already fast enough
  • make sure environments without half-float/half-float-linear extensions fall back gracefully
  • fix flow errors
  • put labels on top in the debug page
  • adjust kernel sizes depending on weight to minimize artifacts
  • Uint8Array -> Uint8ClampedArray WebGL doesn't like it
  • use getPaintValue instead of layer.paint where possible
  • use pixelsToTileUnits
  • extract sourceCache._tiles[parentId] && !sourceCache._coveredTiles[parentId] into a method
  • explain blendFunc(ONE, ONE) in the comments in detail
  • explain the half-float texture with the fallback use in the comments
  • explain the tile flicker hack better in the comments
  • fall back to non-float texture on incomplete framebuffer to make sure heatmaps work on Android
  • add version numbers to spec sdk-support property
  • make sure the docs are great
  • rebase against master again
  • add heatmap render tests

Things I need help with

@anandthakker
Copy link
Contributor

@mourner FYI #5308

@mourner
Copy link
Member Author

mourner commented Sep 21, 2017

Performance is looking pretty good at this point — here's a sample JS profiler breakdown (a heavy clustered heatmaps layer takes only 80% more than the background):

image

"doc": "A heatmap.",
"sdk-support": {
"basic functionality": {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these in this file need a version number before merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I include the number of the next would-be release, e.g. 0.41 here for JS?

"property-function": false,
"transition": true,
"units": "pixels",
"doc": "Heatmap radius.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please improve the documentation for all of the new values in this document?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's still in the checklist as you can see.

this.colorRampData[i + 0] = Math.floor(pxColor[0] * 255 / alpha);
this.colorRampData[i + 1] = Math.floor(pxColor[1] * 255 / alpha);
this.colorRampData[i + 2] = Math.floor(pxColor[2] * 255 / alpha);
this.colorRampData[i + 3] = Math.floor(alpha * 255);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems bogus. getPaintValue colors have components in the range of 0...1, so we have to multiply them with 255 to add them to the texture buffer. Let's take a pxColor value of (1, 1, 1, 0.2). The result R value would be 1 * 255 / 0.1 == 2550, which is beyond the value range of Uint8Array.

It also seems that you're premultiplying the colors here. However, with WebGL, we're using non-premultiplied colors in our RGBImage data structure:

// Not premultiplied, because ImageData is not premultiplied.
// UNPACK_PREMULTIPLY_ALPHA_WEBGL must be used when uploading to a texture.
class RGBAImage {

Instead of manually premultiplying here, we should take the advice and use UNPACK_PREMULTIPLY_ALPHA_WEBGL when uploading later on.

E.g. a color ramp of

"stops": [
    [0, "rgba(255, 0, 0, 0)"],
    [1, "rgba(0, 0, 255, 1)"],
]

only shows blue, and doesn't even show a glimpse of red as it's interpolated from red to blue.

It'd also be cool to have unit tests for generating color ramp textures/buffers from a style specification.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation — I'll look more closely at the premultiplying stuff.

It'd also be cool to have unit tests for generating color ramp textures/buffers from a style specification.

I think it would be enough to cover this with a render test, since the results are visual, not something where a difference would be useful to see as numbers in the console. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Per chat, the code works correctly — leaving it as is as a weird special case because we need to un-premultiply the premultiplied colors returned by getPaintValue because the Texture class later premultiplies them when uploading again.


constructor(layer: LayerSpecification) {
super(layer);
this.colorRampData = new Uint8Array(256 * 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use a Uint8ClampedArray here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interestingly, WebGL doesn't like Uint8ClampedArray as texture input — it produces texture.js:47 WebGL: INVALID_OPERATION: texImage2D: type UNSIGNED_BYTE but ArrayBufferView not Uint8Array.

}
}
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the layer below labels please?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't matter since it's just a debug page. The official example definitely should have labels on top though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but people still look at this, and we should set a good example here.

for (let i = 0; i < coords.length; i++) {
const coord = coords[i];

// skip tiles that have uncovered parents to avoid flickering
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please expand on the reasoning of this a little more? And potentially add a render test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Certainly needs a render test, although it will be tricky to make because of the need to manually make 4 new tile fixtures. The situation here is similar to #5004. The difference is that heatmaps don't need such a complex fix (tile masks) — the difference between tiles of different zooms for heatmaps is much more subtle since it's still vector data, so waiting for all 4 children to load before putting them in place of a parent is an acceptable solution to the problem that's also faster and much simpler.

// large circles are not clipped to tiles
gl.disable(gl.STENCIL_TEST);

renderToTexture(gl, painter, layer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please move this call to a different render pass to improve performance? Extrusion fills use a 3d render pass to render the texture before rendering to the main FBO. See mapbox/mapbox-gl-native#9286 for a description of this optimization, and #5101 for @lbud implemented this for extrusion fills.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kkaefer @lbud should we call this pass differently then? e.g. prerender or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, @mollymerp and I were just talking about this yesterday, as hillshades use a pre-render pass as well — it would make sense to rename the 3d pass to prerender and carefully move relevant parts of the current 3d pass (I suppose for now we could move them into draw_fill_extrusion.js, although eventually if we add other 3D types it'll be moved to a file where it can be shared between types).

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

@mourner some questions and nitpicks inline, but this looks great! I love how simple the code ended up being.

_applyPaintDeclaration(name: any, declaration: any, options: any, globalOptions: any, animationLoop: any, zoomHistory: any) {
super._applyPaintDeclaration(name, declaration, options, globalOptions, animationLoop, zoomHistory);
if (name === 'heatmap-color') {
const len = this.colorRampData.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to happen in _applyPaintDeclaration, or could it happen instead in setPaintProperty()? It looks to me like the latter would work, and I think it'd be preferable to override setPaintProperty rather than the private _applyPaintDeclaration

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually started with setPaintProperty but later moved to this, I don't remember why — will recheck.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I think I remember now — the problem is you can't call getPaintValue immediately after setPaintProperty, because it's being set up in a separate "apply paint transitions" stage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay. Could we add a comment about that here, so that no one is tempted to try such a refactor later on?

const parentTile = sourceCache.findLoadedParent(coord, 0, {});
if (parentTile) {
const parentId = parentTile.coord.id;
if (sourceCache._tiles[parentId] && !sourceCache._coveredTiles[parentId]) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extract this check into one or more SourceCache methods to avoid accessing its private state?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good idea.

const programConfiguration = bucket.programConfigurations.get(layer.id);
const program = painter.useProgram('heatmap', programConfiguration);
programConfiguration.setUniforms(gl, program, layer, {zoom: painter.transform.zoom});
gl.uniform1f(program.uniforms.u_radius, layer.paint['heatmap-radius']);
Copy link
Contributor

Choose a reason for hiding this comment

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

layer.getPaintProperty('heatmap-radius') (similar for other uses of layer.paint)

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean getPaintValue, right? Is there an advantage in case of non-DDS properties? I thought that would be faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep sorry getPaintValue. No advantage for non-DDS, but at some point I think we want to get rid of StyleLayer.{paint,layout} (or hide it), see #3044. I don't think the performance difference would be enough to matter in code that's executed only once per draw.

gl.uniform1f(program.uniforms.u_radius, layer.paint['heatmap-radius']);

const scale = painter.transform.zoomScale(tile.coord.z - painter.transform.zoom);
gl.uniform1f(program.uniforms.u_extrude_scale, EXTENT / tile.tileSize * scale);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is equivalent to pixelsToTileUnits(tile, 1, painter.transform.zoom)


// Kernel density estimation with a Gaussian kernel of size 5x5
float d = -0.5 * 5.0 * 5.0 * dot(v_extrude, v_extrude);
float val = weight * u_intensity * GAUSS_COEF * exp(d);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think having two separate properties for heatmap-intensity and heatmap-weight may be a bit confusing. Is the reason for this in order to decouple the zoom-dependent and feature-dependent dimensions of this multiplier?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. I thought about this a lot and considered making this one property, but it's just too inconvenient to use — you have to construct complex expressions or zoom+property functions and it's just hard to do intuitively. This separation though fits pretty well into how I think about heatmaps. Another more practical benefit is that you can change heatmap intensity in real time without recomputing all the buffers.

Copy link
Contributor

@anandthakker anandthakker Sep 22, 2017

Choose a reason for hiding this comment

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

Yeah, I guess the only alternative would be that the expression would have to be of the form: feature-only-expression or zoom-only-expression or ['*', feature-only-expression, zoom-only-expression]... which is probably just as confusing (or more) as having two properties, heh


// Kernel density estimation with a Gaussian kernel of size 5x5
float d = -0.5 * 5.0 * 5.0 * dot(v_extrude, v_extrude);
float val = weight * u_intensity * GAUSS_COEF * exp(d);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the 5^2 factor here technically translates to a kernel size of 5x5. Here's my read/analysis to make sure I'm understanding this right:

The "bandwidth" of the Gaussian kernel (which is also its standard deviation) would be the value h in 1/2pi * exp( -0.5 * (u/h)^2 )

Since v_extrude is essentially in units of radii (i.e. at a distance of u_radius from the point, v_extrude is a unit vector), the calculation above is equivalent to weight * u_intensity * GAUSS_COEF * exp( (distance_in_pixels / (radius / 5)^ ), making the bandwidth radius/5.

Three standard deviations is a pretty reasonable definition for the practical "size" of a Gaussian kernel. In this case, that would be: 3/5 * radius.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I noted that "radius" is quite a blurry term when referring to kernel density Gaussian kernels, but haven't thought this through further. Given the thinking above, how do you think we should change the formula in the shader?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think changing 5.0 to 3.0 would be pretty reasonable. Then radius would have a pretty clean meaning:

  • For advanced users familiar w/ the underlying math, "3 standard deviations of the Gaussian kernel"
  • For most users, "the distance around each point where it influences the heatmap" (or something like that)

Copy link
Contributor

@anandthakker anandthakker Sep 24, 2017

Choose a reason for hiding this comment

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

Hmm, experimenting with this a bit, one issue is that with if we use 3.0, then for values of intensity * weight > 0.5, the output for a point ends up getting clipped by the square mesh for that point. Whereas with 5.0, it takes intensity * weight > 5000 to make that happen.

I guess the problem is that, even at x = 3 standard deviations, the gaussian curve evaluates to 0.0018, which is over half of the 1/255 resolution of the color ramp.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, now I remember that empirically 5.0 is reasonable enough to make clipping artifacts not to be noticeable. Picking more, the performance will suffer because of the number of fragments. Any ideas on how to do better here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't restrict the weight, otherwise you won't be able to use clustering with heatmaps. If there's a cluster with 1000 points, it's weight should be 1000 times the single point weight.

Copy link
Member Author

Choose a reason for hiding this comment

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

Other option would be to pick another type of kernel (not Gaussian) for the heatmap, or modify the Gaussian to fade out to 0 at the borders regardless of the weight.

Copy link
Member Author

@mourner mourner Sep 24, 2017

Choose a reason for hiding this comment

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

Tried Epanechnikov and the heatmap looks pretty bad. Maybe we could both calculate bandwidth and increase the size of the kernel depending on weight in the vertex shader? That way small intensities wouldn't require as many fragments, which would increase performance, and big intensities which cause big kernels wouldn't appear too often.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mourner Actually if we use a weight-based adjustment to the size of the kernel (i.e., the size of the mesh produced by the vertex shader), then we could go back to just using a simple, fixed value of 3.0 for the bandwidth. I.e., in the vertex shader,

    v_extrude = vec2(mod(a_pos, 2.0) * 2.0 - 1.0) * scale_based_on_weight;

And then in fragment

       float d = -0.5 * h * h * dot(v_extrude / scale_based_on_weight, v_extrude / scale_based_on_weight);

Where scale_based_on_weight is a scale factor that grows the kernel large enough beyond radius so that the kernel falls effectively to zero at the edge

Copy link
Member Author

Choose a reason for hiding this comment

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

@anandthakker exactly, that's what I was thinking too! Now we need to determine that scale properly...

gl.clearColor(0, 0, 0, 0);
gl.clear(gl.COLOR_BUFFER_BIT);

gl.blendFunc(gl.ONE, gl.ONE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add a comment here to explain the significance of this blend function, since it's essential to how the kernel density method works (and maybe some comments in general on this method)

gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.LINEAR);
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.LINEAR);
gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGB, painter.width / 4, painter.height / 4, 0, gl.RGB,
painter.extTextureHalfFloat ? painter.extTextureHalfFloat.HALF_FLOAT_OES : gl.UNSIGNED_BYTE, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of using HALF_FLOAT_OES when it's supported? Is it to increase the precision we use for kde values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Try disabling it and see how the heatmap looks. Better precision allows for a much smoother look.

// Gaussian kernel coefficient: 1 / sqrt(2 * PI)
#define GAUSS_COEF 0.3989422804014327

void main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest renaming this program from heatmap to heatmap_density, and then renaming heatmap_texture to just heatmap

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure those names would be clearer. Also, I named them similarly to the extrusion names — fill_extrusion and extrusion_texture.

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

@mourner this is looking great!

Only one main suggestion: could we change the integration tests to use a simplified geojson source (maybe with, e.g., just 3 or 4 points) rather than a vector tile fixture? I think that would allow the tests to be more easily inspected & evaluate for correctness just by comparing the style definition and the resulting image.

_applyPaintDeclaration(name: any, declaration: any, options: any, globalOptions: any, animationLoop: any, zoomHistory: any) {
super._applyPaintDeclaration(name, declaration, options, globalOptions, animationLoop, zoomHistory);
if (name === 'heatmap-color') {
const len = this.colorRampData.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay. Could we add a comment about that here, so that no one is tempted to try such a refactor later on?

@mourner
Copy link
Member Author

mourner commented Oct 4, 2017

@anandthakker I intentionally used fixtures with a bunch of points instead of a basic GeoJSON to have a good variety of point densities. 3-4 points might not exercise all the edge cases of points overlapping.

BTW, looks like it needs another rebase now :(

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

A couple minor documentation comments. Excited to see this feature come so far along!

@@ -1972,6 +2001,10 @@
"doc": "Gets the current zoom level. Note that in style layout and paint properties, [\"zoom\"] may only appear as the input to a top-level [\"curve\"] expression.",
"group": "Zoom"
},
"heatmap-density": {
"doc": "Gets the density of a pixel in a heatmap layer. Can only be used in `heatmap-color` property.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: add “the” before heatmap-color.

@@ -1972,6 +2001,10 @@
"doc": "Gets the current zoom level. Note that in style layout and paint properties, [\"zoom\"] may only appear as the input to a top-level [\"curve\"] expression.",
"group": "Zoom"
},
"heatmap-density": {
"doc": "Gets the density of a pixel in a heatmap layer. Can only be used in `heatmap-color` property.",
Copy link
Contributor

Choose a reason for hiding this comment

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

As a lay reader, I’d have little idea what it means for a pixel to have density – other than pixel density, which is a different concept. Any idea how to explain density in the context of a heatmap?

[1, "red"]
]
},
"doc": "Defines the color of each pixel based on its density value in a heatmap. Should be either a stop function with input values ranging from `0` to `1`, or a curve expression with a special `[\"heatmap-density\"]` keyword as the input.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that this explanation is a bit specific to the GL JS function and expression syntaxes, so the iOS/macOS SDK will need to override it with the appropriate syntax.

anandthakker added a commit that referenced this pull request Oct 5, 2017
* Add heatmap-density expression (prepares for #5253)

* Accept options for context in which expression is used

Prepares for #5193

* Unify creatExpression and createExpressionWithErrorHandling

Closes #5409

* declaration => property
@@ -45,8 +44,6 @@ function drawCircles(painter: Painter, sourceCache: SourceCache, layer: CircleSt
gl.uniform2fv(program.uniforms.u_extrude_scale, painter.transform.pixelsToGLUnits);
}

gl.uniform1f(program.uniforms.u_devicepixelratio, browser.devicePixelRatio);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this no longer necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an unused leftover — we now define DEVICE_PIXEL_RATIO as a shader constant in the shader generation code.

gl.activeTexture(gl.TEXTURE1);

// Use a 4x downscaled screen texture for better performance
gl.viewport(0, 0, painter.width / 4, painter.height / 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to account for the device pixel ratio here? E.g. do we need a "retina" version of the downscaled texture?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think not because painter.width and painter.height already have "retina" values.

gl.bindFramebuffer(gl.FRAMEBUFFER, fbo);
gl.framebufferTexture2D(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.TEXTURE_2D, texture, 0);

// If using half-float texture as a render target is not supported, fall back to a low precision texture
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can test unavailability of this extension? For VAOs, I think we have code that specifically prevents reporting that this extension is available so that we can run the test with it enabled and disabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately the gl module which we use for tests doesn't seem to support the extension, so we're forced to only test the "fallback" state.

Also, this check is not about availability of the halfFloat extension, it's about being able to use halfFloat textures as a render target (which some GPUs simply don't support while they support the extension). So the only way to know that is to try, and then check the framebuffer attachment status (this is even officially documented somewhere).

Copy link
Member Author

Choose a reason for hiding this comment

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

@mourner mourner merged commit 15a0db7 into master Oct 6, 2017
@mourner mourner deleted the heatmaps branch October 6, 2017 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Heatmap layer type
5 participants