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

Added vibrance filter (for increasing saturation of muted colors) #7189

Merged
merged 5 commits into from
Jul 9, 2021

Conversation

melchiar
Copy link
Member

@melchiar melchiar commented Jul 6, 2021

Added a Vibrance filter for increasing the saturation of an image's more muted colors. Formula based on Caman.js

image

https://jsfiddle.net/melchiar/oq4eydrj/

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2021

Code Coverage Summary

> [email protected] coverage:report /home/runner/work/fabric.js/fabric.js
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.3 |       77 |   85.99 |   83.01 |                                               
 fabric.js |    83.3 |       77 |   85.99 |   83.01 | ...,29780,29905,29985-30050,30173,30272,30489 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2021

Code Coverage Summary

> [email protected] coverage:report /home/runner/work/fabric.js/fabric.js
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.3 |       77 |   85.99 |   83.01 |                                               
 fabric.js |    83.3 |       77 |   85.99 |   83.01 | ...,29781,29906,29986-30051,30174,30273,30490 
-----------|---------|----------|---------|---------|-----------------------------------------------

* Constructor
* @memberOf fabric.Image.filters.Vibrance.prototype
* @param {Object} [options] Options object
* @param {Number} [options.vibrance=0] Vibrance value for the image (between -1 and 1)
Copy link
Member

Choose a reason for hiding this comment

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

ok is written here, but we can put this also over the vibrance property

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@asturur
Copy link
Member

asturur commented Jul 6, 2021

Should we add a test?

@asturur
Copy link
Member

asturur commented Jul 6, 2021

i can setup a test suite for all the filters maybe since you don't have examples.
We could use that image as reference

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2021

Code Coverage Summary

> [email protected] coverage:report /home/runner/work/fabric.js/fabric.js
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.3 |    76.98 |   85.99 |   83.01 |                                               
 fabric.js |    83.3 |    76.98 |   85.99 |   83.01 | ...,29789,29914,29994-30059,30182,30281,30498 
-----------|---------|----------|---------|---------|-----------------------------------------------

@melchiar
Copy link
Member Author

melchiar commented Jul 6, 2021

yeah, I guess we don't really have any visual tests for filters yet, do we

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2021

Code Coverage Summary

> [email protected] coverage:report /home/runner/work/fabric.js/fabric.js
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.3 |    76.98 |   85.99 |   83.01 |                                               
 fabric.js |    83.3 |    76.98 |   85.99 |   83.01 | ...,29789,29914,29994-30059,30182,30281,30498 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2021

Code Coverage Summary

> [email protected] coverage:report /home/runner/work/fabric.js/fabric.js
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.3 |       77 |   85.99 |   83.01 |                                               
 fabric.js |    83.3 |       77 |   85.99 |   83.01 | ...,29797,29922,30002-30067,30190,30289,30506 
-----------|---------|----------|---------|---------|-----------------------------------------------

@@ -15,7 +15,7 @@
* @see {@link http://fabricjs.com/image-filters|ImageFilters demo}
* @example
* var filter = new fabric.Image.filters.Saturation({
* saturation: 100
* saturation: 1
Copy link
Member Author

@melchiar melchiar Jul 6, 2021

Choose a reason for hiding this comment

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

changed this to 1 (100 makes no sense when the recommended range is -1 to 1)

*
* @param {Number} saturation
* @default
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

added a description to the variable saturation filter too since it was missing

@asturur asturur merged commit c36ef82 into master Jul 9, 2021
@asturur
Copy link
Member

asturur commented Jul 9, 2021

I had this idea on my head for a bit:

this code here:

    /**
     * Return WebGL uniform locations for this filter's shader.
     *
     * @param {WebGLRenderingContext} gl The GL canvas context used to compile this filter's shader.
     * @param {WebGLShaderProgram} program This filter's compiled shader program.
     */
    getUniformLocations: function(gl, program) {
      return {
        uVibrance: gl.getUniformLocation(program, 'uVibrance'),
      };
    },

    /**
     * Send data from this filter to its shader program's uniforms.
     *
     * @param {WebGLRenderingContext} gl The GL canvas context used to compile this filter's shader.
     * @param {Object} uniformLocations A map of string uniform names to WebGLUniformLocation objects
     */
    sendUniformData: function(gl, uniformLocations) {
      gl.uniform1f(uniformLocations.uVibrance, -this.vibrance);
    },

is sort of repeated for each filter, but at the end is conceptually:

uniformsValeus = {
  uVibrance: function (filter) { return -filter.vibrance };
}

We still need a smart way to identify the type of the filter, in this case 1f

All the rest is wrapping code that could be in the base filter class.
i'm sure this would make the filters more easy to read and maybe save some byte here and there.

If not all, at least `getUniformLocation can be streamlined to a data declaration.

@melchiar melchiar deleted the vibrance-filter branch July 19, 2021 22:34
@asturur asturur mentioned this pull request Aug 27, 2021
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.

2 participants