Decluttering mode by style #13566

cns-solutions-admin · 2022-04-12T06:01:08Z

As requested e.g. in #7475, this improves decluttering by being able to state per (image) style, how this style should be decluttered:

declutter => declutter as before
obstacle => draw image/text, but use it as obstacle for styles with mode "declutter"
none => do not declutter

Decluttering still only happens, if the layer also has declutter = true.

cns-solutions-admin · 2022-04-12T06:24:45Z

I named the property declutterMode, because

it strongly hints that it is only used, if the layer's declutter = true
it is easier to understand than mapbox's allowOverlap and ignorePlacement
we can easily add other modes later on

I have also implemented support for decluttering in ol-mapbox-style. PR will follow, when this is accepted.

cns-solutions-admin · 2022-04-12T09:01:56Z

Build errors (doc type links) fixed.

github-actions · 2022-04-14T18:23:52Z

📦 Preview the examples and docs from this branch here: https://deploy-preview-13566--ol-site.netlify.app/.

ahocevar

Thanks for your work on this, @cns-solutions-admin!

Instead of introducing a DeclutterMode text enum, you could simply create a typedef with a union type. Something like:

/** @typedef {"none" | "obstacle" | "declutter"} DeclutterMode */

Eventually, we'll be changing all string enums in the library to union types.

To make sure that this new feature can be maintained, it would be important to add tests. In this case, two rendering tests (one for each new mode) would be appropriate.

Of course it would be nice to have this new option also for Text styles, but that does not need to be part of this pull request.

Finally, would you be able to squash the commits in a way that the pull request does not contain any merge commits? A single commit for the whole pull request would also work.

cns-solutions-admin · 2022-04-20T16:28:22Z

I succeeded in squashing everything into one commit.
I added a rendering test for both modes based on the other decluttering test.
I left the DeclutterMode enum to be consistent with the other enums - unless everyone uses typescript, this way is safer.

ahocevar · 2022-04-21T20:29:50Z

Thanks for your continued effort on this, @cns-solutions-admin.

I left the DeclutterMode enum to be consistent with the other enums - unless everyone uses typescript, this way is safer.

We'll be removing these enums, see #12696. We do have the tooling in place (npm run typecheck) to ensure correct values with union types, and the generated d.ts files do not work with enums. For example, the return type for the getDeclutterMode() API method you introduced, will be broken from the start if you stick with the enum.

ahocevar · 2022-04-21T20:48:16Z

The rendering test does not test the difference between none and obstacle mode. If there were e.g. other features rendered on top of the red and blue circles, it could be shown that with none mode (on top of blue) the other features would be drawn, but not with obstacle mode (on top of red).

cns-solutions-admin · 2022-04-25T11:55:34Z

Based on your comments "you could..." and "eventually, ...", I assumed that it was better to use the enum for now.
It's changed now and the test is also improved.

ahocevar

In ahocevar@bbd58ea, I tried to make the test more readable. And by doing so, I saw something I have no explanation for:

Why are the red circles on top of the c-orange label?

ahocevar · 2022-05-02T08:04:42Z

test/rendering/cases/layer-vector-decluttering-extended/main.js

+  new Feature({
+    geometry: new Point([center[0], center[1]]),
+    text: 'center',
+    zIndex: 2,


No need to set a zIndex property on any of the features in this test - you're not using them in the style.

You introduced a typo: "zindex" instead of "zIndex". Thus the red circles were not used for the orange labels, as those were on top of the red layer.

But this highlighted that I somehow did not commit the continue in the executor: the obstacles are used in the declutter executor to be added to the declutter tree, but rendered in the normal executor - so that they are behind the labels (in the test they were also rendered in the declutter executor thus being drawn on top of the label).

Fixed the executor and updated the test with your version + fix + additional blue circle/text to test

that objects (red) cannot be obstacles for layers with a higher z index (blue)

that the obstacles (red) are drawn behind layers with a higher z index (blue)

Allows to specify for each image style, whether the image should be decluttered, always drawn but still serving as obstacle, or drawn without being an obstacle for other images/texts. The layer must still have declutter = true set for this property to have any effect.

ahocevar

Yes, the new behavior is what I would expect. I added two more comments regarding the tests. When these are addressed, I think this is ready for prime time.

ahocevar · 2022-05-05T11:06:44Z

test/rendering/cases/layer-vector-decluttering-extended/main.js

+  }),
+]);
+// circles are always drawn,
+// texts are decluttered against each other and the layer 1 circles/texts


For readability, please refer to the color instead of the layer number everywhere.

comments improved.

ahocevar · 2022-05-05T11:12:39Z

test/rendering/cases/layer-vector-decluttering-extended/main.js

+const sourceRed = new VectorSource();
+sourceRed.addFeatures([
+  new Feature({
+    geometry: new Point([center[0] + 1000, center[1] + 1000 - 200]),


The previous version with the overlapping red circles was better, to show that they do serve as obstacle, but are rendered even if they overlap.

ok, reverted zoom, moved orange circles' text to left with xOffset to show at least one label.

ahocevar · 2022-05-05T11:39:10Z

test/rendering/cases/layer-vector-decluttering-extended/main.js

+// circles are always drawn, but serve as obstacles,
+// texts are decluttered against each other and the circles


I think this comment is not applicable. declutterMode declutter here, so if you had more blue circles overlapping this one, they wouldn't be rendered.

removed when improving comments.

ahocevar

Thanks for your effort on this, @cns-solutions-admin

Rendering test mismatch

ahocevar · 2022-05-05T12:00:50Z

Now we have a rendering test mismatch. The actual result does not show the e-cyan label for the cyan layer:

cns-solutions-admin · 2022-05-05T12:24:53Z

This is weird. Locally the test works.

ahocevar · 2022-05-05T12:53:28Z

I doubt that a difference in font rendering would cause this label to disappear, but I'll try in interactive mode to see what happens when zooming in/out a bit.

ahocevar · 2022-05-05T14:14:27Z

This is really weird. The results are quite stable when zooming in/out locally.

ahocevar · 2022-05-05T14:25:55Z

@cns-solutions-admin Maybe you could cherry-pick 134462c, so we can see if the tests pass then. Or you could grant maintainers access to this pull request's branch, then I could push that change here directly.

cns-solutions-admin · 2022-05-06T04:58:35Z

Done. Seems that the font on my computer is a bit smaller/slimmer.

ahocevar · 2022-05-06T14:28:50Z

Now it looks good. You have to update the reference image in your branch with the one from the CI, or give me access to this pull request's branch, or cherry-pick 9d17d92.

cns-solutions-admin · 2022-05-06T14:56:32Z

CI image downloaded, committed and pushed.
Seems that giving access to branches is not possible for organization accounts.

ahocevar

Thanks for your continued effort on this, @cns-solutions-admin

marcjansen · 2022-05-06T20:52:46Z

Very nice collaboration here.

ahocevar requested changes Apr 17, 2022

View reviewed changes

cns-solutions-admin force-pushed the decluttering branch from fadad3e to 13e3a12 Compare April 20, 2022 15:38

cns-solutions-admin force-pushed the decluttering branch from 8108f1f to 596f817 Compare April 21, 2022 12:35

cns-solutions-admin force-pushed the decluttering branch from cf9b60a to 8b380aa Compare April 25, 2022 11:47

ahocevar reviewed May 5, 2022

View reviewed changes

cns-solutions-admin force-pushed the decluttering branch from 4874dc8 to e1593ce Compare May 5, 2022 10:35

ahocevar reviewed May 5, 2022

View reviewed changes

improve test with overlapping circles.

9338286

ahocevar reviewed May 5, 2022

View reviewed changes

improve test with overlapping circles.

9adfe7a

ahocevar previously approved these changes May 5, 2022

View reviewed changes

Zoom out a bit for rendering test

bce37b5

reference image from CI

6ab4d45

ahocevar approved these changes May 6, 2022

View reviewed changes

ahocevar merged commit 71f3780 into openlayers:main May 6, 2022

cns-solutions-admin mentioned this pull request May 16, 2022

set declutter mode openlayers/ol-mapbox-style#561

Merged

renovate bot mentioned this pull request Aug 28, 2022

Update all minor versions (master) (minor) camptocamp/ngeo#8794

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decluttering mode by style #13566

Decluttering mode by style #13566

cns-solutions-admin commented Apr 12, 2022

cns-solutions-admin commented Apr 12, 2022

cns-solutions-admin commented Apr 12, 2022

github-actions bot commented Apr 14, 2022

ahocevar left a comment

cns-solutions-admin commented Apr 20, 2022

ahocevar commented Apr 21, 2022 •

edited

Loading

ahocevar commented Apr 21, 2022

cns-solutions-admin commented Apr 25, 2022

ahocevar left a comment

ahocevar May 2, 2022

cns-solutions-admin May 5, 2022

ahocevar left a comment

ahocevar May 5, 2022

cns-solutions-admin May 5, 2022

ahocevar May 5, 2022

cns-solutions-admin May 5, 2022

ahocevar May 5, 2022

cns-solutions-admin May 5, 2022

ahocevar left a comment

ahocevar commented May 5, 2022 •

edited

Loading

cns-solutions-admin commented May 5, 2022

ahocevar commented May 5, 2022

ahocevar commented May 5, 2022

ahocevar commented May 5, 2022

cns-solutions-admin commented May 6, 2022

ahocevar commented May 6, 2022

cns-solutions-admin commented May 6, 2022

ahocevar left a comment

marcjansen commented May 6, 2022

		// circles are always drawn, but serve as obstacles,
		// texts are decluttered against each other and the circles

Decluttering mode by style #13566

Decluttering mode by style #13566

Conversation

cns-solutions-admin commented Apr 12, 2022

cns-solutions-admin commented Apr 12, 2022

cns-solutions-admin commented Apr 12, 2022

github-actions bot commented Apr 14, 2022

ahocevar left a comment

Choose a reason for hiding this comment

cns-solutions-admin commented Apr 20, 2022

ahocevar commented Apr 21, 2022 • edited Loading

ahocevar commented Apr 21, 2022

cns-solutions-admin commented Apr 25, 2022

ahocevar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahocevar 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

ahocevar left a comment

Choose a reason for hiding this comment

ahocevar commented May 5, 2022 • edited Loading

cns-solutions-admin commented May 5, 2022

ahocevar commented May 5, 2022

ahocevar commented May 5, 2022

ahocevar commented May 5, 2022

cns-solutions-admin commented May 6, 2022

ahocevar commented May 6, 2022

cns-solutions-admin commented May 6, 2022

ahocevar left a comment

Choose a reason for hiding this comment

marcjansen commented May 6, 2022

ahocevar commented Apr 21, 2022 •

edited

Loading

ahocevar commented May 5, 2022 •

edited

Loading