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

Ensure consistent label positioning by sorting LabelProviders #60346

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

Conversation

PatrikSylve
Copy link

Problem

I have a QGIS project, containing vector data with rule base labeling that i publish in WMS in QGIS Server. When requesting a map, the labels will render on different positions each request. So the output image will vary even though the input is remains the same.

Example, two identical GetMap requests generating different output (notice the labels for example Oslo & Göteborg):

map_label1
map_label2

Solution

The issue stemmed from features being processed in seemingly random orders during each run of Pal::extractProblem. This was due to Pal::mLayers storing LabelProviders in an unordered_map.

To resolve this, I:

  • Change the type from unordered_map to map
  • Added a custom comparator to order QgsAbstractLabelProvider by providerId, or layerId as fallback.

Related issue

This potentially resolves #28174

Note

The issue is still present when having a vector tile web service as data source. In that case one soulution was to sort the layer->mFeatureParts before iterating the features in Pal::extractProblem.

    std::sort( layer->mFeatureParts.begin(), layer->mFeatureParts.end(), []( const std::unique_ptr<FeaturePart> &a, const std::unique_ptr<FeaturePart> &b ) {
      return a->featureId() < b->featureId();
    } );

But I suspect the feature order is influenced by the order the tiles are received from the vector tile service, so this may require additional handling specific to that use case.

@github-actions github-actions bot added this to the 3.42.0 milestone Jan 29, 2025
Copy link

github-actions bot commented Jan 29, 2025

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 2cc1c22)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 2cc1c22)

@nyalldawson
Copy link
Collaborator

@PatrikSylve GREAT fix -- this will make a lot of people happy!

Are you confident fixing the test failures here?

@PatrikSylve
Copy link
Author

Thanks, glad to hear!

I am not familiar with the test suite, but I will look into it and give it a try

@PatrikSylve
Copy link
Author

@nyalldawson

Got the tests up and it would fail occasionally due to the layerId being generated randomly. Assigned a custom ID for the layers in the test and it seems to succeed consistently.

@nyalldawson
Copy link
Collaborator

@PatrikSylve I have an alternative idea -- see #60369. Instead of storing the layers in a map in the first place, we can just use a vector. This ensures that the layers will ALWAYS be processed in their actual layer order from the project, so things are no longer dependent on the layer ID (which, as you've noted, is itself non-deterministic).

I'm keen to hear your thoughts!

@PatrikSylve
Copy link
Author

@nyalldawson

I continued on your work in #60369 and attempted to also use vector instead of map for QgsRuleBasedLabelProvider::mSubProviders. This also ensured that the subProviders were handled in consistent order when having rule base labeling.

What do you think?

@@ -43,7 +43,7 @@ class CORE_EXPORT QgsRuleBasedLabeling : public QgsAbstractVectorLayerLabeling
public:
class Rule;
typedef QList<QgsRuleBasedLabeling::Rule *> RuleList;
typedef QMap<QgsRuleBasedLabeling::Rule *, QgsVectorLayerLabelProvider *> RuleToProviderMap;
typedef std::vector<std::pair<QgsRuleBasedLabeling::Rule *, QgsVectorLayerLabelProvider *>> RuleToProviderMap;
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 move this to private? it shouldn't be exposed to the python bindings

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.

Deterministic label placement
2 participants