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

Translate qgis::fieldcalculator to C++ #38695

Merged
merged 5 commits into from Sep 11, 2020

Conversation

suricactus
Copy link
Contributor

@suricactus suricactus commented Sep 11, 2020

The qgis:fieldcalculator algorithm is currently written in Python and consists of rather custom UI. This PR attempts to translate the algorithm to a C++ implementation, aiming at speed, compatibility.

The UI slightly changes from the previous implementation, having to custom UI widgets. There is a bit of inconvinience that both "Result in" fields are optional, but I don't find a way to make them either required. This is well noted in the algorithm description.

before after new field
Screenshot_20200909_230218 image

A simpler version of #38675 , that targets only the algorithm.

@github-actions github-actions bot added this to the 3.16.0 milestone Sep 11, 2020
Copy link
Member

@m-kuhn m-kuhn left a comment

Choose a reason for hiding this comment

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

Given the improvements of a stable C++ algorithm over the python one and the previous implementation in #38675 being discouraged at the moment, I am in favor of merging this one with the minimal UI change for now and continuing the discussion for an improved user interface in parallel.

Are there any tests for this algorithm already?

@suricactus
Copy link
Contributor Author

I have added an additional test. Good you mentioned the tests, it was not exactly backwards compatible. Now all should be good.

@m-kuhn m-kuhn merged commit 3bea2e7 into qgis:master Sep 11, 2020
INPUT:
name: points.gml
type: vector
NEW_FIELD_NAME: test
Copy link
Member

Choose a reason for hiding this comment

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

@suricactus the old algorithm used FIELD_NAME as input, is there a safe way to have this backwards compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you encounter an issue? This should keep the backwards compatibility. The old test is passing too?
https://github.com/qgis/QGIS/pull/38695/files#diff-53600f4cb9f762166bd6d0f4d56f555cR125-R135

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.

None yet

2 participants