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

Added *-sort-key support #1060

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

orangemug
Copy link
Contributor

This PR implements the *-sort-key rules.

Here is an example with the following layer rule, where #ff0000 fills have a higher sort order that #0000ff fills.

{
  "id": "test",
  "type": "fill",
  "source": "polygons",
  "layout": {
    "fill-sort-key": [
      "get",
      "zIndex"
    ]
  },
  "paint": {
    "fill-color": [
      "get",
      "color"
    ]
  }
}

Previously this resulted in

Screenshot from 2023-12-14 16-20-25

Now we get the correct result.

Screenshot from 2023-12-14 16-21-24

@orangemug
Copy link
Contributor Author

@ahocevar this failing test seems to be caused by a flakey test.

I can get this to pass/fail locally by just running npm test multiple times. Any ideas?

@ahocevar
Copy link
Member

The test issue should have been fixed a few days ago. Maybe just rebase this pull request?

@orangemug
Copy link
Contributor Author

This branch is all up to date and has the latest commit 4d4aeaa (on main)

# git status | grep "On branch"
On branch feature/add-sort-key-support
# git log --oneline | grep 4d4aeaa
4d4aeaa Merge pull request #1050 from openlayers/dependabot/npm_and_yarn/html-webpack-plugin-5.5.4
# git push origin feature/add-sort-key-support
Everything up-to-date

@ahocevar
Copy link
Member

At first glance it looks to me like the tests fail correctly because this pull request breaks something. But I don't have time at the moment to give it a closer look.

@ahocevar
Copy link
Member

ahocevar commented Dec 16, 2023

I see what's wrong. The current sort (index) makes sure layers as defined in the Mapbox style are rendered in the correct order (i.e. the order they appear in the layers array). This pull request breaks that.

If I read the *-sort-key Property correctly, it should sort the features so they appear in the correct order within the layer. To achieve that, the features should be sorted using the renderOrder function of the ol/layer/Vector. I think the renderOrder function for ol/layer/VectorTile only sorts within tiles and not across tiles, but I'm not sure.

@orangemug
Copy link
Contributor Author

I'll have another proper look tomorrow @ahocevar. But FYI I still see flakiness with tests. I get passes/fails of the test suite on repeat runs without any changes

# npm test
Chrome Headless 119.0.6045.105 (Linux x86_64): Executed 128 of 176 SUCCESS (2.07 secs / 1.928 secs)
TOTAL: 128 SUCCESS

=============================== Coverage summary ===============================
Statements   : 62.41% ( 890/1426 )
Branches     : 62.91% ( 570/906 )
Functions    : 74.11% ( 126/170 )
Lines        : 63.09% ( 865/1371 )
# echo $?
0

@orangemug
Copy link
Contributor Author

I think within the layer is working correctly, at least it should be will confirm tomorrow. The function uses a secondary index multiplied by a tiny number.

export function calcSortIndex(index, sortIndex) {
  // The `sortIndex` here is the index within the layer, we multiply that by
  // a tiny number so we end up with a small offset within the current layers
  // bounds
  return index + (sortIndex === undefined ? 0 : sortIndex) * 0.00000000001;
}

@ahocevar
Copy link
Member

Using a different zIndex for every feature adds a significant performance penalty. Using the layer's renderOrder function would be a better choice.

@orangemug
Copy link
Contributor Author

Using a different zIndex for every feature adds a significant performance penalty. Using the layer's renderOrder function would be a better choice.

Ok cool, will change over tomorrow.

Regarding tests @ahocevar. I'm seeing some sort of odd object mutation thing (I think) this line seems to differ on some runs of the test suite, propertySpec is sometimes undefined. I'll see if I can work what's going on.

value = propertySpec.default;

@orangemug orangemug marked this pull request as draft December 18, 2023 11:01
@orangemug orangemug marked this pull request as ready for review December 18, 2023 12:13
@orangemug
Copy link
Contributor Author

orangemug commented Dec 18, 2023

I hope this is what you expected @ahocevar

It seems to work as I expect (although I found a different rendering issue #1063 on main), let me know what you think.

@ahocevar
Copy link
Member

I'm currently reworking decluttering and z-index ordering in OpenLayers. Let's revisit when that is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants