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

Allow expressions in LABEL PRIORITY #6884

Merged
merged 3 commits into from
May 29, 2023

Conversation

geographika
Copy link
Member

@geographika geographika commented May 25, 2023

This allows expressions to be added to a LABEL PRIORITY As well as attribute bindings and integer values.

This is useful to allow labels to be prioritised on attributes dynamically e.g. calculate a 1-10 value based on population sizes of cities (by calculating a percentage of the highest value).

The code is based on the SLD pull request from @jbo-ads.

There are also comments from @sdlime on the pull request about using this as a template pull request to allow expressions for other Mapfile values.

Tests have been added for each of the 3 ways of setting a label priority - numeric value, attribute binding, and the new expression option.

Code reviews would be appreciated. I'm unsure why the following block is required (the SIZE pull request included this). Is it to clean-up expressions if a Mapfile is read a second time?

    if (label->exprBindings[MS_LABEL_BINDING_PRIORITY].string) {
        msFreeExpression(&label->exprBindings[MS_LABEL_BINDING_PRIORITY]);
        label->nexprbindings--;
    }

Docs to be updated if merged.

@jmckenna jmckenna added this to the 8.2.0 Release milestone May 26, 2023
@rouault
Copy link
Contributor

rouault commented May 26, 2023

LGTM

I'm unsure why the following block is required (the SIZE pull request included this). Is it to clean-up expressions if a Mapfile is read a second time?

I believe this is just to avoid memory leaks if the PRIORITY keyword would appear several times in the same LABEL object.
(by the way my understanding of the work done by msFreeExpression() is that the .string member is freed and reinitialize to NULL, so the explicit msFree( .string) before label->exprBindings[MS_LABEL_BINDING_PRIORITY].string = msStrdup(msyystring_buffer) is very likely useless, but better keep it for consistency with similar code paths)

@geographika
Copy link
Member Author

I believe this is just to avoid memory leaks if the PRIORITY keyword would appear several times in the same LABEL object. (by the way my understanding of the work done by msFreeExpression() is that the .string member is freed and reinitialize to NULL, so the explicit msFree( .string) before label->exprBindings[MS_LABEL_BINDING_PRIORITY].string = msStrdup(msyystring_buffer) is very likely useless, but better keep it for consistency with similar code paths)

Thanks @rouault for the review and clarification on the above - most appreciated.
I aim to add expression support for STYLE->SIZE in a future pull request.

@geographika
Copy link
Member Author

Just for reference @rouault is correct that the following block is used to clean-up any duplicate PRIORITY in a label:

    if (label->exprBindings[MS_LABEL_BINDING_PRIORITY].string) {
        msFreeExpression(&label->exprBindings[MS_LABEL_BINDING_PRIORITY]);
        label->nexprbindings--;
    }

In the case of a Mapfile containing two PRIORITY keywords the first is ignored (and cleaned-up with the above code), and the second expression is used by the label. I added this as part of the msautotest.

LABEL
  PRIORITY ([priority] * 2)
  PRIORITY (10 - [priority])
  ...        
END

@geographika geographika merged commit db74212 into MapServer:main May 29, 2023
rouault pushed a commit that referenced this pull request Jun 22, 2023
This pull request allows the new functionality in #6884 of using an expression in a LABEL PRIORITY in MapScript. 
It takes a similar approach to setting attribute bindings for properties, which requires using the appropriate attribute constant such as `MS_LABEL_BINDING_PRIORITY`:

```python
label = mapscript.labelObj()
label .setExpressionBinding(mapscript.MS_LABEL_BINDING_PRIORITY, "[MY_ATTRIBUTE] * 2")
```

This is a little clunky but is at least consistent with setting attribute bindings:

```python
label = mapscript.labelObj()
label.setBinding(mapscript.MS_LABEL_BINDING_COLOR, "NEW_BINDING")
```

While testing this I noticed that the `writeLabel` function used to write Mapfile objects ignored expressions. This seems an oversight also for the `LABEL` `SIZE` expression. If this approach seems appropriate I can use this for other expression bindings.

Something to note is that a property could have an attribute binding **and** an expression binding set. In this case the attribute binding currently takes precedence when calculating a shape value, so it also takes precedence when writing to a Mapfile. 

In theory setting an expression could also set the attribute binding to NULL. This is not currently the case for MapScript or when reading a Mapfile twice.
jmckenna pushed a commit to jmckenna/MapServer that referenced this pull request Aug 3, 2023
…Server#6904)

This pull request allows the new functionality in MapServer#6884 of using an expression in a LABEL PRIORITY in MapScript. 
It takes a similar approach to setting attribute bindings for properties, which requires using the appropriate attribute constant such as `MS_LABEL_BINDING_PRIORITY`:

```python
label = mapscript.labelObj()
label .setExpressionBinding(mapscript.MS_LABEL_BINDING_PRIORITY, "[MY_ATTRIBUTE] * 2")
```

This is a little clunky but is at least consistent with setting attribute bindings:

```python
label = mapscript.labelObj()
label.setBinding(mapscript.MS_LABEL_BINDING_COLOR, "NEW_BINDING")
```

While testing this I noticed that the `writeLabel` function used to write Mapfile objects ignored expressions. This seems an oversight also for the `LABEL` `SIZE` expression. If this approach seems appropriate I can use this for other expression bindings.

Something to note is that a property could have an attribute binding **and** an expression binding set. In this case the attribute binding currently takes precedence when calculating a shape value, so it also takes precedence when writing to a Mapfile. 

In theory setting an expression could also set the attribute binding to NULL. This is not currently the case for MapScript or when reading a Mapfile twice.
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.

3 participants