Implemented ScatterplotLayer #124

ilijapuaca · 2020-05-02T07:34:32Z

Two things that I had to cover that are specific to this layer:

It's using layer uniforms in the fragment shader, so I've added support for specifying the stage uniforms should be bound in
It was using triangle fan which doesn't seem to be supported in WebGPU. I've converted the points so that they define the same geometry using triangle strips

That being said I don't think these look the way they're supposed to look, they seem to be squares instead of circles that I'm seeing on the web. Also, when zooming in very closely, they seem to stop scaling at some point. I'm seeing weird geometry in the geometry inspector (could just be a bug in the tool, it's pretty flimsy sometimes) although all of the values seem valid, no NaNs or anything.

I also did not test the line component, are there any examples that make use of it?

ibgreen

Very nice, this is progressing really fast now!

ibgreen · 2020-05-02T16:43:52Z

cpp/examples/deck.gl/flight-paths.cc

+auto createScatterplotLayer(const std::string &dataPath) -> std::shared_ptr<ScatterplotLayer::Props> {
+  auto scatterplotLayerProps = std::make_shared<ScatterplotLayer::Props>();
+  scatterplotLayerProps->id = "airports";
+  scatterplotLayerProps->getPosition = [](const Row &row) -> mathgl::Vector3<float> {


Use return type deduction to minimize the amount of boilerplate

auto [](const Row &row) { return row.getVector3<float>("coordinates"); };

The method is already calling out the return type (which is also why I was softly lobbying for templates, responding to question in another PR).

If we just call the helper variable props we may be able to fit it into one line, which would increase readability (at least according to my personal aestethics).

ibgreen · 2020-05-02T16:49:58Z

cpp/modules/deck.gl/layers/src/line-layer/line-layer.h

@@ -67,10 +67,10 @@ class LineLayer::Props : public Layer::Props {
    return new LineLayer{std::dynamic_pointer_cast<LineLayer::Props>(props)};
  }

-  std::string widthUnits{"pixels"};                         // 'pixels',
-  float widthScale{1};                                      // {type: 'number', value: 1, min: 0},


Nit: We haven't captured the min values in the prop types, so we can't check those.

How should we deal with these kind of ranges in general? Is there a way to get these clamped implicitly without relying on a backing type?

ibgreen · 2020-05-02T16:54:52Z

cpp/modules/deck.gl/layers/src/scatterplot-layer/scatterplot-layer.cpp

@@ -140,6 +114,9 @@ void ScatterplotLayer::initializeState() {
  auto lineWidth = std::make_shared<arrow::Field>("instanceLineWidths", arrow::float32());
  auto getLineWidth = std::bind(&ScatterplotLayer::getLineWidthData, this, std::placeholders::_1);
  this->attributeManager->add(garrow::ColumnBuilder{lineWidth, getLineWidth});
+
+  // TODO(ilija@unfolded.ai): Where should we initialize models?


Yes, creating Models in initializeState is what we do. Because at this time, the layerContext has been assigned so WGPU device etc is available at this time.

ibgreen · 2020-05-02T16:56:23Z

cpp/modules/deck.gl/layers/src/scatterplot-layer/scatterplot-layer.cpp

+  auto lineWidthScale = props->lineWidthScale * widthMultiplier;
+
+  ScatterplotLayerUniforms layerUniforms{
+      props->opacity, props->radiusScale,        props->radiusMinPixels,    props->radiusMaxPixels,


Long lists of positional parameters of same type (float etc) looks very error prone as the compiler can only do limited checking.

Either use the C++20 designated initializers or assign one by one?

ibgreen · 2020-05-02T16:57:54Z

cpp/modules/deck.gl/layers/src/scatterplot-layer/scatterplot-layer.cpp

+  for (auto const& model : this->getModels()) {
+    // Layer uniforms are currently bound to index 1
+    model->setUniforms(
+        std::make_shared<garrow::Array>(this->context->device, &layerUniforms, 1, wgpu::BufferUsage::Uniform), 1);


This feels like we are creating and destroying a lot of GPU buffers.

Fine for now just to get rendering working, but longer term, GPUBuffers should be populated in updateState if possible so that they are ready to just set here.

Moved buffer creation, data setting and binding across initialize/update/draw state methods.

We now create a single buffer that gets filled with data, which works well except that updateState isn't called when viewport updates (is that supposed to be the case?), so widthMultiplier isn't correct.

Temporarily made it so that updateState is called from drawState, which can easily be removed once diffing/dirty flags work correctly

We now create a single buffer that gets filled with data, which works well except that updateState isn't called when viewport updates (is that supposed to be the case?), so widthMultiplier isn't correct.

I suggest we design this somewhat independently from JS. The JS API didn't moved all uniform updating into updateState. It also implements the "uniform animation" feature in a JavaScript dependent way.

Updating when viewport changes can trigger a lot of updates, e.g. when using multiple views we get multiple updates every frame. So we want to minimize calls if possible.

Not sure I fully understand where we should be doing the updates then, let's sync on this one

ibgreen · 2020-05-02T17:00:28Z

cpp/modules/luma.gl/core/src/model.cpp

@@ -152,7 +152,7 @@ auto Model::_createBindGroupLayout(wgpu::Device device, const std::vector<Unifor
    -> wgpu::BindGroupLayout {
  std::vector<wgpu::BindGroupLayoutBinding> bindings;
  for (uint32_t i = 0; i < uniforms.size(); i++) {
-    auto binding = wgpu::BindGroupLayoutBinding{i, wgpu::ShaderStage::Vertex, wgpu::BindingType::UniformBuffer,
+    auto binding = wgpu::BindGroupLayoutBinding{i, uniforms[i].shaderStage, wgpu::BindingType::UniformBuffer,


I wonder if we could just default this to always assign to both, so apps don't need to track this? Or do we get warnings and inefficiencies if we assign it but then it is not used?

I'm not what the implications of doing that would be, if there's an additional overhead (I assume there is, else it would be done by default?) I'd prefer we declare it explicitly. If not we can of course get rid of it

ibgreen · 2020-05-02T17:07:19Z

That being said I don't think these look the way they're supposed to look, they seem to be squares instead of circles that I'm seeing on the web. Also, when zooming in very closely, they seem to stop scaling at some point. I'm seeing weird geometry in the geometry inspector (could just be a bug in the tool, it's pretty flimsy sometimes) although all of the values seem valid, no NaNs or anything.

An initial key observation is that the "circles" in the scatterplot layer are created in the fragment shader, not the vertex shader. I.e. we do not have a geometry that approximates a circle. We have a square geometry, and we discard any pixels that are at length(position) > 1 (or position * position > 1).

It is then worth verifying that this calculation/approach is still working here - the code looks OK from a quick scan, but worth poking around commenting things out etc and make sure it has the intended effect.

Also, a trivial mistake could have been made when converting the geometry. It should be a "unit" square.

I also did not test the line component, are there any examples that make use of it?

Assuming you are talking about the outline around the circles, that is a fairly recent addition to the scatterplotlayer and I don't know if any examples that use it (@Pessimistress may know more)

ilijapuaca · 2020-05-04T08:40:36Z

Addressed pending comments. Spent some time debugging potential issues with circle rounding, it seems to be a precision problem, where we'd end up with a larger radius than we're supposed to, leaving us with a circle that isn't rounded perfectly.

Changing SMOOTH_EDGE_RADIUS in geometry.glsl.h to 0 seems to completely get rid of the issue. I am not clear on the specifics about smoothedge/smoothstep, I see that it's a way to interpolate values, but I am not sure why it'd cause results we're seeing when it has the same value in JS.

I am not sure what other implications updating that value might have, so I left it as-is, but that's the only thing I found that looked suspicious. Will revisit based on feedback there, and merge this to avoid code pilling up

ilijapuaca requested review from ibgreen and isaacbrodsky May 2, 2020 07:34

ilijapuaca force-pushed the feature/scatterplot-layer branch from ef25af7 to 6ea58da Compare May 2, 2020 07:34

Implemented ScatterplotLayer

cbe4516

ilijapuaca force-pushed the feature/scatterplot-layer branch from 6ea58da to cbe4516 Compare May 2, 2020 12:16

ibgreen approved these changes May 2, 2020

View reviewed changes

ilijapuaca added a commit that referenced this pull request May 4, 2020

#124 comments addressed

6e8cfe6

#124 comments addressed

35be8bc

ilijapuaca force-pushed the feature/scatterplot-layer branch from 6e8cfe6 to 35be8bc Compare May 4, 2020 08:39

ilijapuaca merged commit 3611925 into master May 4, 2020

ilijapuaca mentioned this pull request May 4, 2020

ScatterplotLayer circles not rounded correctly #126

Closed

ilijapuaca deleted the feature/scatterplot-layer branch May 8, 2020 06:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented ScatterplotLayer #124

Implemented ScatterplotLayer #124

ilijapuaca commented May 2, 2020

ibgreen left a comment

ibgreen May 2, 2020

ibgreen May 2, 2020

ilijapuaca May 4, 2020

ibgreen May 2, 2020

ibgreen May 2, 2020

ibgreen May 2, 2020

ilijapuaca May 4, 2020

ibgreen May 4, 2020

ilijapuaca May 5, 2020

ibgreen May 2, 2020

ilijapuaca May 4, 2020

ibgreen commented May 2, 2020

ilijapuaca commented May 4, 2020

Implemented ScatterplotLayer #124

Implemented ScatterplotLayer #124

Conversation

ilijapuaca commented May 2, 2020

ibgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen commented May 2, 2020

ilijapuaca commented May 4, 2020