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
start to work on new branch for conditional statement #4
base: master
Are you sure you want to change the base?
Conversation
… case-insensitive IF/if/If/iF
@@ -42,7 +44,8 @@ class ANALYSIS_EXPORT QgsRasterCalcNode | |||
tOperator = 1, | |||
tNumber, | |||
tRasterRef, | |||
tMatrix | |||
tMatrix, | |||
tFunct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's rename this to a full name - tFunction
- this is preferred in qgis code
@@ -74,7 +77,7 @@ class ANALYSIS_EXPORT QgsRasterCalcNode | |||
opABS, | |||
opMAX, | |||
opMIN, | |||
opNONE, | |||
opNONE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can keep the trailing comma :-)
* Calculates result of raster calculation when tFunct type is used | ||
* \since QGIS 3.22 | ||
*/ | ||
QgsRasterMatrix evaluation( const QVector<QgsRasterMatrix *> &matrixVector, QgsRasterMatrix &result ) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- let's rename the function to something more descriptive - e.g.
evaluateFunction
- to make it clear this is used for functions - make the function private - there's no need to have it in public api
//name of the function | ||
QString name( "if" ); | ||
|
||
QgsRasterCalcNode node( name, args ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that parsing of "if" condition works, it will be better to use the parser instead of manually creating the expression - it will be also much easier to read/understand the test(s)
m1.setValue( 1, 0, 13.0 ); | ||
m1.setValue( 1, 1, -1.0 ); //nodata | ||
m1.setValue( 2, 0, 11.0 ); | ||
m1.setValue( 2, 1, 1.0 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you use the same test raster block multiple times, it would be good to move the raster block initialization to a dedicated function in the test (to avoid code duplicity)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see what you mean. Taking into account the last two comments I decided to chage the test method a little bit. I'll skip the creation of the raster and use existing raster data (such as landsat.tif
), if this last path is not clear or is not more understandable, I'll get back to the first approach..
I also add more complex arguments as options.
matrixContainer.append( singleMatrix.release() ); | ||
} | ||
|
||
//result = evaluation( matrixContainer, result ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented code
…m/Franc-Brs/QGIS into rastercalc-conditional-statements
@@ -42,7 +44,8 @@ class ANALYSIS_EXPORT QgsRasterCalcNode | |||
tOperator = 1, | |||
tNumber, | |||
tRasterRef, | |||
tMatrix | |||
tMatrix, | |||
tFuncttion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tFuncttion | |
tFunction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, unable to write defined-by-me type correctly
Description
[Replace this with some text explaining the rationale and details about this pull request]