-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Heatmap layer #11046
Heatmap layer #11046
Conversation
Looks like the current build issues on iOS can be fixed by adding |
src/mbgl/gl/context.hpp
Outdated
@@ -144,6 +144,8 @@ class Context : private util::noncopyable { | |||
return { size, createTexture(size, nullptr, format, unit) }; | |||
} | |||
|
|||
UniqueTexture createTexture(Size size, const void* data, TextureFormat, TextureUnit); |
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.
I'd prefer if we kept this method private and use a more high level createTexture
call instead.
if (isUndefined(value)) { | ||
return HeatmapColorPropertyValue(); | ||
} else if (isExpression(value)) { | ||
optional<std::unique_ptr<Expression>> expression = convert<std::unique_ptr<Expression>>(value, error, expression::type::Color); |
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 the unique_ptr
in this optional be ever be empty? If so, we should check for that to avoid null dereferences further down.
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.
It should never be null (we use the optional to represent parsing failure). It's a bug rather than a recoverable failure if the pointer is null, so if we want to check here, it should be with an assert
} // namespace conversion | ||
} // namespace style | ||
} // namespace mbgl | ||
|
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.
Nit: stray whitespace
include/mbgl/style/layer_type.hpp
Outdated
@@ -13,6 +13,7 @@ enum class LayerType { | |||
Background, | |||
Custom, | |||
FillExtrusion, | |||
Heatmap |
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.
Nit: let's add trailing commas
@@ -414,6 +414,7 @@ global.describeValue = function (value, property, layerType) { | |||
} | |||
return displayValue; | |||
case 'color': | |||
if (property.name === 'heatmap-color') value = 'red'; |
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.
TODO: remove this before merging
platform/darwin/src/MGLStyle.mm
Outdated
@@ -214,7 +216,7 @@ - (MGLSource *)memberOfSources:(MGLSource *)object { | |||
- (MGLSource *)sourceWithIdentifier:(NSString *)identifier | |||
{ | |||
auto rawSource = self.rawStyle->getSource(identifier.UTF8String); | |||
|
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.
Let's remove all of these whitespace changes. The convention for Objective-C files (and Xcode's default formatting behavior) is to indent empty lines like the last block.
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.
Is there any way to format whitespace of a whole file according Xcode conventions? Adding those back by hand will be super-tedious... (update: nevermind, updated manually since most of the updated files have mixed indentation)
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.
Editor ‣ Structure ‣ Re-Indent (⌃I) reindents the selection. You can use it on wide swaths of the code, but probably not the entire file, because the file contains some hand-indented macros.
platform/node/src/node_map.cpp
Outdated
@@ -529,7 +530,7 @@ void NodeMap::release() { | |||
uv_close(reinterpret_cast<uv_handle_t *>(async), [] (uv_handle_t *h) { | |||
delete reinterpret_cast<uv_async_t *>(h); | |||
}); | |||
|
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.
Let's try to remove whitespace changes to make this PR less unwieldy.
scripts/style-spec.js
Outdated
@@ -1,5 +1 @@ | |||
var spec = module.exports = require('../mapbox-gl-js/src/style-spec/reference/v8'); | |||
|
|||
// Make temporary modifications here when Native doesn't have all features that JS has. |
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.
Please keep this comment
} else { | ||
return *it->second.statistics<Property>().max(); | ||
} | ||
} |
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.
We don't need this function
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.
What is the function for, and why do we need it for circle layer then? The code isn't very clear about its purpose...
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.
In CircleBucket
, it's an internal helper that's used by getQueryRadius
. You're right, it should have a clarifying comment.
|
||
if (!colorRampTexture) { | ||
const auto colorRampSize = Size{256, 1}; | ||
colorRampTexture = gl::Texture{colorRampSize, parameters.context.createTexture(colorRampSize, colorRamp.data(), gl::TextureFormat::RGBA, 1)}; |
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.
Let's use context.createTexture(...)
instead of directly initializing a gl::Texture
object
|
||
const style::HeatmapLayer::Impl& impl() const; | ||
|
||
std::array<uint8_t, 1024> colorRamp; |
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.
Let's use an AlphaImage
instead of a plain std::array
. The image stores it's dimensions internally and allows us to use it without changing the Context::createTexture
API. This is the same pattern that we're also using for the LineAtlas:
const AlphaImage image; |
mapbox-gl-native/src/mbgl/geometry/line_atlas.cpp
Lines 124 to 132 in f075cdb
void LineAtlas::upload(gl::Context& context, gl::TextureUnit unit) { | |
if (!texture) { | |
texture = context.createTexture(image, unit); | |
} else if (dirty) { | |
context.updateTexture(*texture, image, unit); | |
} | |
dirty = false; | |
} |
|
||
You can set this property to an expression containing any of the following: | ||
|
||
* Constant numeric values |
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.
FYI, the next time we merge the release-boba branch to master, we’ll need to rerun make darwin-style-code
so that #11018 can mention the minimum value here.
* Interpolation and step functions applied to the `$heatmapDensity` variable | ||
|
||
This property does not support applying interpolation or step functions to | ||
feature attributes. |
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.
Apparently this property does support data-driven styling, but the codegen script inserts this line because property-function
is false
for heatmap-color
. Should we special-case heatmap-color
?
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.
We resolved this (I think in chat), but just noting here for reference that heatmap-color does not, in fact, support data-driven styling, so the above is correct.
try { | ||
renderTexture->bind(); | ||
} catch (const std::runtime_error& ex) { | ||
// can't render to a half-float texture; falling back to unsigned byte one |
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 context.supportsHalfFlooatTextures
be updated in this case, so we don't need to have this kind of expensive try..catch
in other parts of the code.
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.
Yes! What's the best way to do it? Add a setter like disableHalfFloatTextures()
, or maybe expose supportsHalfFloat
as a public property?
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.
I don't think a disableHalFloatTextures()
setter makes sense. It may be better to make the supprtsHalfFloat
member variable public, similar to whats done in
mapbox-gl-native/src/mbgl/gl/context.hpp
Lines 291 to 298 in fcc3808
public: | |
// For testing and Windows because Qt + ANGLE | |
// crashes with VAO enabled. | |
#if defined(_WINDOWS) | |
bool disableVAOExtension = true; | |
#else | |
bool disableVAOExtension = false; | |
#endif |
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.
@asheemmamoowala how does 3726c58 look?
@@ -369,6 +376,9 @@ | |||
55D9B4B01D005D3900C1CCE2 /* libz.tbd */ = {isa = PBXFileReference; lastKnownFileType = "sourcecode.text-based-dylib-definition"; name = libz.tbd; path = usr/lib/libz.tbd; sourceTree = SDKROOT; }; | |||
55E2AD101E5B0A6900E8C587 /* MGLOfflineStorageTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = MGLOfflineStorageTests.mm; path = ../../darwin/test/MGLOfflineStorageTests.mm; sourceTree = "<group>"; }; | |||
55FE0E8D1D100A0900FD240B /* config.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; name = config.xcconfig; path = ../../build/macos/config.xcconfig; sourceTree = "<group>"; }; | |||
89462398200D199100DA8EF2 /* heatmap.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = heatmap.json; sourceTree = "<group>"; }; |
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.
FYI, this file isn’t used anywhere. Did you want help hooking up a command for displaying heatmaps in macosapp, or did you only intend for the style to be used with the Custom Style dialog? (wms.json is included for the same reason, so that would be fine.)
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.
Exactly, same case as wms.json
.
9264e27 adds an icon to macosapp’s Layers sidebar for heatmap layers: |
METHOD(&HeatmapLayer::getHeatmapIntensity, "nativeGetHeatmapIntensity"), | ||
METHOD(&HeatmapLayer::getHeatmapOpacityTransition, "nativeGetHeatmapOpacityTransition"), | ||
METHOD(&HeatmapLayer::setHeatmapOpacityTransition, "nativeSetHeatmapOpacityTransition"), | ||
METHOD(&HeatmapLayer::getHeatmapOpacity, "nativeGetHeatmapOpacity")); |
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.
heatmap color seems to be missing from above list. Will look into why this is missing (seems to be supported on iOS though)
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.
@tobrun I disabled any code generation for heatmap-color
on the Android side because it requires being able to pass expressions as its values — as far as I remember, we had some bits missing about expressions which made it very hard to make it work.
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.
thank you for the context, will ticket it for now. Update: ticket here.
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.
Nit: can we rearrange includes etc. so that the layer order is always the same? E.g. raster, hillshade, heatmap, ... should always be in the same order.
@@ -28,6 +28,7 @@ var layers = Object.keys(spec.layer.type.values).map((type) => { | |||
}, []); | |||
|
|||
const paintProperties = Object.keys(spec[`paint_${type}`]).reduce((memo, name) => { | |||
if (name === 'heatmap-color') return memo; |
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 please create a ticket that tracks removal of this special casing?
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.
The ticket is here #11172
@@ -79,6 +82,11 @@ | |||
"doc": "The base color of this layer. The extrusion's surfaces will be shaded differently based on this color in combination with the `light` settings. If this color is specified with an alpha component, the alpha component will be ignored; use `fill-extrusion-opacity` to set layer opacityco." | |||
} | |||
}, | |||
"paint_heatmap": { | |||
"heatmap-color": { | |||
"doc": "Defines the color of each pixel based on its density value in a heatmap. Should be an expression that uses `$heatmapDensity` as input." |
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.
Nit: remove double space
* Special-case implementation of (a subset of) the PropertyValue<T> interface | ||
* used for building the HeatmapColor paint property traits class. | ||
*/ | ||
class HeatmapColorPropertyValue { |
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.
Instead of naming this after heatmaps, can we find a more generic name for this in light of other code that might use these color ramps too?
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.
Yes! In fact @lbud's line gradient branch, which reuses it, renames it to ColorRampPropertyValue
. However I'd leave it as is for now to be consistent with GL JS, and only rename when we land another feature that uses it such as line gradients.
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.
🔥 Looking great!
A couple of questions below, but deferring to your judgment on them @mourner
* Interpolation and step functions applied to the `$heatmapDensity` variable | ||
|
||
This property does not support applying interpolation or step functions to | ||
feature attributes. |
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.
We resolved this (I think in chat), but just noting here for reference that heatmap-color does not, in fact, support data-driven styling, so the above is correct.
namespace heatmap { | ||
MBGL_DEFINE_UNIFORM_SCALAR(float, u_extrude_scale); | ||
} // namespace heatmap | ||
|
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.
Other program-specific uniforms are defined in xxx_program.hpp
-- should we move this one to heatmap_program.hpp
?
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.
I would leave it here until we refactor #11026 so that it's clear why we namespace it and that there's a collision.
|
||
if (layer.is<RenderHeatmapLayer>()) { | ||
layer.as<RenderHeatmapLayer>()->updateColorRamp(); | ||
} |
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.
This means we're updating the color ramp every time a heatmap-color layer changes, even if the change was to some other property. Should we check the diff more granularly here?
Closes #10146. Remaining things:
OES_texture_half_float
,OES_texture_half_float_linear
andEXT_color_buffer_half_float
extensions where available)Add support for runtime-styled heatmap color on Androidon hold for nowImage
class for creating color ramp texture