Linear transform #1024
Linear transform #1024
Conversation
There was a problem hiding this comment.
Thanks for this PR Marcel, looks good. I've left a small comment inline plus there are some linter errors:
lib/operation.js:434:2: Unnecessary semicolon.
test/unit/linear.js:79:3: Block must not be padded by blank lines.
test/unit/linear.js:81:1: Too many blank lines at the end of file. Max of 0 allowed.
lib/operation.js
Outdated
| } else if (is.number(b)) { | ||
| this.options.linearB = b; | ||
| } else { | ||
| throw new Error('Invalid linear transform multiplier ' + b); |
Cleanup code
src/pipeline.h
Outdated
| @@ -159,6 +161,8 @@ struct PipelineBaton { | |||
| thresholdGrayscale(true), | |||
| trimTolerance(0), | |||
| gamma(0.0), | |||
| linearA(1.0), | |||
There was a problem hiding this comment.
Just spotted that these fields need to be initialised in the same order as they are declared to avoid a compiler warning.
../src/pipeline.h:83:10: warning: ‘PipelineBaton::gamma’ will be initialized after [-Wreorder]
double gamma;
^
../src/pipeline.h:81:10: warning: ‘double PipelineBaton::linearA’ [-Wreorder]
double linearA;
^
There was a problem hiding this comment.
I like to change this - but how do i actually see these compiler warnings. I use npm install to build a new release (unfortunately under windows) and there i can't see these warnings.
(I have to admit that I'm not programming cpp for so long.)
There was a problem hiding this comment.
Thanks for updating - MSVC doesn't appear to warn about this but gcc and clang do.
There was a problem hiding this comment.
|
Thanks, I'll look at merging this after the suit v0.19.0 WIP branch makes its way into master. |
|
@3epnm Hello, there's a small conflict in the |
|
I have a look |
package.json
Outdated
| @@ -39,6 +39,7 @@ | |||
| "Guy Maliar <[email protected]>", | |||
| "Nicolas Coden <[email protected]>", | |||
| "Matt Parrish <[email protected]>", | |||
| "Marcel Bretschneider <[email protected]>" | |||
There was a problem hiding this comment.
I think a trailing comma is required here.
|
I made the required change in the package.json |
|
Thank you very much Marcel, your work will be included in the next release. I notice from your profile that you're based in London so you may be interested in #1104 |
Hello,
i have done some refactoring for PR at #742 including the suggested changes. However, i have created a new branch with a fresh fork from the master branch in my account.
rfc.