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.

Sign up for GitHub

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

Jump to bottom

[mvt] mapbox vector tiles conversion improvements #58154

Merged
merged 9 commits into from
Jul 24, 2024
Merged

[mvt] mapbox vector tiles conversion improvements #58154

merged 9 commits into from
Jul 24, 2024

Conversation

3nids
Copy link
Member

@3nids 3nids commented Jul 17, 2024 •
edited
Loading

  • fix brush when color is data defined
  • fix match expression when single values are used
  • fix stops are open on both ends

from this style (https://vectortiles.geo.admin.ch/styles/ch.swisstopo.lightbasemap.vt/style.json):

     "paint": {
        "fill-color": [
          "match",
          [
            "get",
            "luminosity"
          ],
          -15,
          "rgb(200,210,213)",
          -14,
          "rgb(203,213,216)",
          -13,
          "rgb(207,215,218)",
          -12,
          "rgb(210,218,221)",
          -11,
          "rgb(213,221,224)",
          -10,
          "rgb(217,224,226)",
          -9,
          "rgb(220,227,229)",
          -8,
          "rgb(224,230,231)",
          -7,
          "rgb(227,232,234)",
          -6,
          "rgb(231,235,237)",
          -5,
          "rgb(234,238,239)",
          -4,
          "rgb(238,241,242)",
          -3,
          "rgb(241,244,245)",
          -2,
          "rgb(245,247,247)",
          -1,
          "rgb(248,249,250)",
          "rgb(252, 252, 252)"
        ],

@github-actions github-actions bot added this to the 3.40.0 milestone Jul 17, 2024
@3nids 3nids requested a review from nyalldawson July 17, 2024 13:51
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 17, 2024 •
edited
Loading

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 98924d7)

@3nids 3nids changed the title [mvt] fix brush when color is data defined [mvt] fix brush when color is data defined and match expression Jul 17, 2024
nyalldawson
nyalldawson reviewed Jul 17, 2024
View reviewed changes
src/core/vectortile/qgsmapboxglstyleconverter.cpp
@@ -453,7 +456,7 @@ bool QgsMapBoxGlStyleConverter::parseFillLayer( const QVariantMap &jsonLayer, Qg
fillSymbol->setStrokeStyle( Qt::NoPen );
}

if ( fillColor.isValid() )
if ( fillColor.isValid() || colorIsDataDefined )
{
fillSymbol->setFillColor( fillColor );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If fillColor isn't valid, then we'll be setting the fill symbol to an invalid QColor here... I don't think we want to do that!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you do? Move the check and do an else if?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what it's trying to do here? I think ideally it'd be set to eg the first colour in the list

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just trying to avoid to set "no brush" if it's data defined. So not setting it might be sufficient.

@nyalldawson
Copy link
Collaborator

Can you add tests for this?

@3nids 3nids force-pushed the 3nids-patch-1 branch 2 times, most recently from f716a9a to 6b078b8 Compare July 20, 2024 15:09
@3nids
Copy link
Member Author

3nids commented Jul 22, 2024

tests added

@3nids
Copy link
Member Author

3nids commented Jul 23, 2024

@nirvn last commit 0c0b085 is a fix on parseArrayStops which you were over some time ago

@3nids 3nids changed the title [mvt] fix brush when color is data defined and match expression [mvt] mapbox vector tiles conversion improvements Jul 24, 2024
nirvn
nirvn approved these changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@nirvn nirvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

@3nids 3nids merged commit 398b151 into master Jul 24, 2024
30 checks passed
@3nids 3nids deleted the 3nids-patch-1 branch July 24, 2024 13:18
@qgis-bot qgis-bot mentioned this pull request Jul 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyalldawson nyalldawson nyalldawson left review comments

@nirvn nirvn nirvn approved these changes

Assignees
No one assigned
Labels
Vector tiles
Projects
None yet
Milestone
3.40.0
Development

Successfully merging this pull request may close these issues.
3 participants
@3nids @nyalldawson @nirvn