New paint property fill-comp-op for fill type layer #1191
Conversation
BenchmarksTest1:
Test2:
All reports in PDF:About the performance results of HillshadeLoadThe scores were tested between product and my local main branch. And the time cost is close to the one of feature branch. So my PR doesn't bring any regression for HillshadeLoad.
|
|
Bundle size report: Size Change: +335 B
ℹ️ View Details
|
tongust
force-pushed
the
fill-opacity-per-layer
branch
from
f20692a
to
9916f5b
Compare
tongust
changed the title
tongust
changed the title
tongust
changed the title
|
Looks interesting, thanks. Can you post the full CLI benchmark results? And can you ping me on slack? |
Updated all benchmark results in above Benchmarks section. |
|
I'm not sure the name is intuitive, also I'm not sure I understand what it should solve. |
This pr is an attempt to address the legacy issue of mapbox
And note that the per-layer opacity only supports fill type for now. For line and other type, they are in my TODO list. |
|
@HarelM I think this feature is very important especially for lines as the current rendering is often not the desired effect. We have for example the use case of a semi-transparent street-network overlay over satellite images. currently intersections are two dark because multiple streets overlap. There are a lot of use-cases with images in multiple mapbox issues which would benefit from this. @tongust It was my understanding that this is difficult to achieve in a performant way. Do you have any input on possible performance impact of this pull request with and without enabling the feature? I personally am not sure about the coverage of the benchmarks, but this seems as this should be covered so your change seems to not have any impact on existing styles. Is this result expected? P.S. HillshadeLoad seems completely unproblematic and also almost certainly not connected to this pull request anyway. The difference just looks big in the graphic because this is a slow benchmark in general. |
|
|
Thanks for the information!
That's it for now, thanks a lot for all the hard work! |
Sorry for the delayed reply. 1. About the designSame to heatmap and hillshade types, one frame buffer object is created for fill to render to offscreen (not canvas actually). As we know, in painter, the offscreen render pass is prior to others (opaque/translucent... passes). For per-geometry opacity, there is no offscreen pass. Each geometry is drawn into canvas (screen) using alpha blending. For per-layer opacity, there are two render passes. First one is offscreen pass. The color mode of rendering is set to
For example, there are two overlapping polygons red A and blue B. B is on the top of A. Their opacity is 0.5, and the background color is In per-geometry opacity, the process is:
In per-layer opacity, the process is:
2. Sampler test casesHere are the cases (per-geometry opacity):
3. Data driven impact.Good points! There was one bug here and now it is fixed. The opacity and color can be changed by data driven properties in per-layer opacity. In below case, the opacity of left green geometry is 0.9 while the right one is 0.4. 4. unit test.Yes, the test draw_fill.test.ts is added. |
|
Regarding the style spec changes. #48 seems to be related (correct me if I'm wrong). I personally don't know about the use cases for other blending modes but this seems the right time to discuss if we ever want to support other blending modes. In that case we have the option to make an enum setting instead of a boolean to add different blending modes. |
|
@birkskyum @JannikGM @xabbu42 can any of you review the sharers part? Does it affect maplibre-native? |
tongust
force-pushed
the
fill-opacity-per-layer
branch
from
d0c7c8d
to
1939f09
Compare
@xabbu42 @HarelM |
tongust
changed the title
tongust
changed the title
|
I agree this PR should not be blocked just for supporting other layer types like line or more blending modes. The current limitations can be documented and we can add more support later without any backward compatibility problems. If features as described in #48 will be possible and valuable in the future though, we should use an enum instead of a bool for the style spec already in this PR so that it is easier to add more blending modes later without any compatibility issues or inflating the spec with lots of not combinable bool settings. Unfortunately I don't have the necessary knowledge to review the shader part. |
|
I'm not sure how urgent this is. We can make PRs against this branch to better define the spec possibilities. I prefer to take the time and define the spec well as it will be very hard to change it in the future... |
@HarelM It is not urgent 😊. Agree with you to define the spec to make it more extensible and maintainable. |
|
I opened an issue in native to coordinate using the latest shaders, see maplibre/maplibre-native#293 |
|
I believe this is good now. I'm happy with the new names. |
|
What does source mean in this? I personally would assume the source to be the complete layer and destination to be the map, and that also seems to be how carto uses it. But this patch conceptually does not change how the layer gets composited on the map, instead it changes how the features of the layer are composited on the layer itself. So source here is a single line feature and destination is the layer before composing it on the map. At least to me this names are therefore confusing. P.S. You are certainly right that implementation of other modes does not need to be part of this PR. Unfortunately for style spec changes we do need to already think about possible future features to avoid problems with backward compatibility in the future. |
Thanks @HarelM The per-line support is on my radar and I will start to work on it. And the feature will be added into another new PR (not mixed with this one), is that okay for you? |
@xabbu42 The source means the pixel to be drawn into the screen or offscreen in WebGL context (see https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/blendFunc). For composting each geometry of fill, the source means the latest geometry to be blended with rest geometries. The destination means the composted geometries, but it does not mean the map. So the The name |
|
Sure, per line can be a postponed to a different PR, but if it's something that can be added soon I think it would better to wait and merge them together and release a version that had them both. My 2 cents anyway... |
|
Thanks for the detailed explanations @tongust. So my confusion was not unfounded. If we want to be flexibel in the future and support all manner of blending combinations it would make sense to have But if we only want to ever support some combinations with known use-cases I would prefer one property without the word operation (as the property potentially influences multiple different operations), something like I personally would have an obscure use case for Just my 2 cents anyway. |
|
I wonder if we can mark certain style spec features as experimental/use at your own risk. Then we could just go forward which changes like this, even include them in releases, without committing to support them exactly as they are implemented now. |
|
Maybe we can use [Experimental] in the deception so that it will be written to the docs as well? |
|
Sounds like a good idea. It is really hard to never break the style spec but I think this should exactly be our goal... Mapbox aimed to never break the style spec, see mapbox/mapbox-gl-js#6584 |
|
I made an issue to move this forward. I don't think this should block on that issue though. Imho it is enough to just document the new features in this branch as experimental as @HarelM suggested to merge it. We can add all the necessary code changes for experimental features retroactively after #1307 is resolved. |
|
@xabbu42 where are we on this? Is the only pending comment is about the names? |
|
I tested the new debug page locally and it behaves as I would expect. As said I can't review the shader part. From my side the only thing missing is to document that this features are experimental and subject to change. Then we can merge this and postpone the decision on the final style spec. |
@HarelM I am tring to extend the per-layer feature to the line, but unfortunately it is not easy job because gl codes of draw line are more complicate than ones of draw fill. Maybe we could split it out into another PR. So if you agree not to wait for supporting line, the only pending comments are about the naming and claiming @xabbu42 Thanks for your suggestions, I think it may be better to have below changes:
|
|
Things to consider are:
|
|
@tongust: Just for clarification: the suggestions of feature-over and layer-over are meant as values for a single property blending-mode where there is more than one source and a value like source-over would not be clear. For the separate property fill-feature-composite-operation source-over is perfectly fine and it would be silly to repeat the feature part which is already in the property name. |
|
I think we wanted to publish the link to the Technical Steering Committee Meeting in the slack channel, if it is not there, feel free to ping me... Everyone is welcome to join this meeting. This meeting takes place once a month and the next one is scheduled for Wednesday, July 13th, 2022, 20:05 Zurich time. |
Proposal based on "Rust ideas": Essentially every, addition, removal or change to the style specification corresponds to a new version of the style spec. For example the addition of a stable feature would mean that we go from If you want to use an unstable feature of the style specification then you have to specify it in the {
"version": 8,
"name": "OSM Liberty",
"unstable-features": ["fill-comp-op", ...]
...
}That way you have to opt-into unstable features in your style specification. unstable features are only available if you use a nightly build of maplibre (can also be done runtime). If you use this @nyurik thats basically rust nightly/unstable features in maplibre :D |
|
I was thinking more in the direction of a js property - i.e.: import maplibre from 'maplibre-gl'
maplibre.useExperimetalFeatures = true;Not sure I have a good reason as to why besides the fact that I want to change the spec as little as possible :-) |
|
I'm surprised I didn't comment here yet (I probably did on Slack?). I feel this is a bit of a dangerous PR, because it can significantly raise rendering cost for a seemingly trivial property. Maybe the shown approach is the more mapbox-ish thing to do, however, what I propose as an alternative is to add a new layer type (for advanced users) which can be used to split the layer stack into framebuffers. (Click here if you want to see my approach to this problem)My proposed alternative Introduce new layer types for starting rendering to framebuffer (setting a framebuffer) + stop rendering to framebuffer (resetting to default screen framebuffer) + blitting framebuffers. Example (layers from bottom to top):
What this means When rendering, the framebuffer would be set by the "cache-set" type layer. This essentially creates layer-groups. This would make maplibre similar to how layers work in graphics editing software, where you can group a bunch of layers, and then composite them differently. There are at least 2 good strategies for the framebuffer: screen-space and tiles. Since terrain was added, something similar must already exist, too. Because for terrain you also need to render layers into a draping texture (ideally that is also tiled - I don't remember how that works in maplibre) for draping. That said, it might be a bit tricky to make this work in the existing ecosystem, like CustomLayers, Heatmap or Hillshade, which already do something similar internally (but I think it's still very doable?). Design choices Aside from the layer types I mention here, it could also be parameterized differently, such as a "group-start" layer type which sets a framebuffer and a "group-end" layer type which would switch to the screen framebuffer and automatically also blit the current group. There might also be choices how nested groups would work or if they'd be allowed. For 3D scenes it might be cool if the depth buffer remained the same, so where you place your group would suddenly be relevant (as you could place your layers infront of buildings [group before extruding buildings, blit after buildings], or behind buildings [group after rendering extruded buildings, blit after buildings]). Also wether the the previous frame (or multiple?) would be cached etc. might be relevant (and if you could draw before writing to the framebuffer) . Such things could be used for effects like motion-blur. How to solve the original issue with my proposal?
This is also similar to how this is implemented in this PR - except I propose that this technical detail is exposed for advanced user. In fact, I did something similar for deck.gl (although it's quite hacky there, too). deck.gl is slightly better suited for it, because it has a layer hierarchy, rather than a list, and it takes whatever argument types you want (including WebGL textures/framebuffers). More importantly I don't need to modify deck.gl itself to implement this, I can just create my own layer outside of the deck.gl codebase. That way, we can render some layers blurred (regardless of their type), or recolorize layers (including animations) without recomputing their entire content. This is all very promising, but the issue with deck.gl is that I don't know what other layers do internally, and they also directly operate on WebGL, so a layer in a group can corrupt the "LayerGroup" renderer; also some built-in features of deck.gl are unaware of my cache. Although, take this with a grain of salt, because I'm probably thinking about this a bit too technically (being a graphics programmer myself). |
|
@JannikGM there's a |
I don't think you can implement my proposal (or this PR) in a custom-layer. A custom-layer is merely an implementation of the (abstracted and simplified) mapbox layer interface where you get to run your own WebGL commands (or whatever else you want to do) when your layer gets rendered - however, it has no context about the layers which come before or after it, so it can't affect them. That a layer has side-effects on other layers (as in my propsal) would also be something very new (hence, why I called the current PR more mapbox-ish).
To be clear: the performance risk only arises when people turn on the compositing mode for a fill layer, because at that point you end up with switching the framebuffer, which can be a costly operation. You also have to keep an extra framebuffer, for a 1080p resolution, that would likely be (2x 1920x1080 x 4 ~ 16MiB of video memory). The more serious issue with this PR (in my opinion), is that the implementation is in the fill layer, and not generic enough to be reusable with other layers or groups of layers. The solution to that is what I proposed, or at least moving it to the render-loop (similar to how we do terrain rendering probably). I'm also fine with tongust approach, but I feel like we should implement it in the rendering loop (= so you could also have a source-comp operation on a symbol-layer for example) and not in each layer individually (leading to code duplication if we want to support other layers). For terrain rendering, we already render layers offscreen (= to a texture) in the main render loop, which is then draped over a terrain; so I assume that ingredients to this already exist. See this bit of code in the layer rendering loop which handles terrains: maplibre-gl-js/src/render/painter.ts Line 474 in f1252a0 Because it's so similar to a sub-problem of terrain rendering, I'd also be curious what @prozessor13 thinks (who is probably also most familiar with the revised maplibre layer rendering loop). |
|
I did not digg in all details of this PR, and i can not say too much about gl-performance, but as @JannikGM already said, it is a similar scenario of rendering terrain. I think this feature would be very easy to integrate into the terrain-logic. Terrain-code currently already splits style-layers into stacks. e.g. a set of combinable layers which should be rendered to texture. So with some additions:
there would be (only in terrain3d) a general solution possible. Next. Your implementation, same as heatmap, is not compatible with 3d, because all is rendered to one big framebuffer, instead of rendered to tiles. Hillshade, in contrast to heatmap, creates a separate FBO (with a separate texture for each FBO) for every tile, and i think this is the way to go to work in 2d and 3d. Overall i think this is a great feature, i would needed such a feature more than one time. To go around of this lack of functionality i always had to combine areas in vectortiles. possible, but a lot of work. |
|
This has gone stale a bit, and we also moved the style-spec logic out of this repo. |
|
Not feeling competent enough to push this forward, but it would be amazing to this feature implemented one way or another. My use case is I'm drawing a As I understand, such problems can be solved by the proposed feature. 
|
Description
This PR is to add new paint property opacity-per-layer to draw fill type layer.
By setting fill-opacity, multiple polygons can be drawn with transparency for data visualization. But in some scenarios, the fill-opacity is misleading.
With this change, the opacity of whole layer is fixed and the color will always be same to one of the top most shape.
Note:
Launch Checklist
maplibre-gl-jschangelog:<changelog></changelog>.