-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
chore(TS): BREAKING move also defaults for filters away from prototype #8742
Conversation
Build Stats
|
the fact that all the code i added saved 1.3kb leaves me a bit confused. |
Ok this is ready to go. |
This is ready to go i think. |
oh no controls yet. |
I will review now. |
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.
This is a great cleanup! Files are so much readable and less intimidating (for me they were, WebGL still is)
- naming: can we keep camelCase or UpperFirst convesion?
colorMatrix.shaders
=>ColorMatrixShaders
, or perhaps create a folderShaders
and move all of them there. I like that you took it out and I like the idea of having them in aShaders
folder. They are consts so seems right to me. - moving
fromObject
to base class withnew this
will save a lot of lines, there are only 2-4 filters that override it - extract the rest of WebGL c code (is it c?) from filter files
import { T2DPipelineState, TWebGLUniformLocationMap } from './typedefs'; | ||
import { classRegistry } from '../ClassRegistry'; | ||
import { blendColorFragmentSource } from './blendColor.shaders'; | ||
|
||
type TBlendMode = |
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.
I think we should export most types in general
|
||
export const blurDefaultValues: Partial<TClassProperties<Blur>> = { | ||
blur: 0, | ||
mainParameter: 'blur', |
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.
shouldn't this be static as well?
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.
i don't want to waste time on this, every test is time consuming between changes, test fixes and surprises.
Not all classes have a mainParameter, and unless we codify it as none
or something like that, is just a bunch of different cases.
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.
(i already tried 3 times to remove it entirely or reduce its appareances)
src/filters/ColorMatrixFilters.ts
Outdated
export function createColorMatrixFilter(key: string, matrix: number[]) { | ||
return class GeneratedColorMatrix extends ColorMatrix { | ||
/** | ||
* Filter type | ||
* @param {String} type | ||
* @default | ||
*/ | ||
type = key; | ||
|
||
/** | ||
* Colormatrix for the effect | ||
* array of 20 floats. Numbers in positions 4, 9, 14, 19 loose meaning | ||
* outside the -1, 1 range. | ||
* @param {Array} matrix array of 20 numbers. | ||
* @default | ||
*/ | ||
matrix = matrix; | ||
|
||
/** | ||
* Lock the matrix export for this kind of static, parameter less filters. | ||
*/ | ||
mainParameter = undefined; | ||
const newClass = class extends ColorMatrix { | ||
get type() { | ||
return key; | ||
} | ||
|
||
/** | ||
* Lock the colormatrix on the color part, skipping alpha | ||
*/ | ||
colorsOnly = true; | ||
static defaults = { | ||
...colorMatrixDefaultValues, | ||
/** | ||
* Lock the matrix export for this kind of static, parameter less filters. | ||
*/ | ||
mainParameter: undefined, | ||
matrix, | ||
}; | ||
|
||
static async fromObject(object: any) { | ||
return new GeneratedColorMatrix(object); | ||
return new this(object); | ||
} | ||
}; | ||
classRegistry.setClass(newClass, key); | ||
return newClass; | ||
} |
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.
I would make this not a mixin but a subclass
See https://github.com/fabricjs/fabric.js/blob/08e9431b6c19f763216f9061d7cec25774c58961/src/filters/ColorMatrixFilters.ts
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.
mixins suck in TS but that is not the only reason. The the class name is correct ad it reduces complexity of code as well.
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.
but this is not a mixin. this is a subclass generated with a function.
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.
I meant dynamic classes, sorry
fragmentSourceTOP: ` | ||
precision highp float; | ||
uniform sampler2D uTexture; | ||
uniform vec2 uDelta; | ||
varying vec2 vTexCoord; | ||
`, |
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.
for consistency should be moved to a standalone file?
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.
this resize one is too messy. but shouldn't be a property either, i will move this
`, | ||
}; | ||
|
||
type TResizeType = 'bilinear' | 'hermite' | 'sliceHack' | 'lanczos'; |
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.
export
when you do this make sure to spread the object and remove |
No is not happening because the constructor is avoing getting type in. |
i can move them in a folder yes, that will make the .shader. go away |
is shader language, not sure the exact definition. |
There are additional things that can be done here:
|
Motivation
Fortunately the defaults values for filters aren't important at all.
The only real change is the fragmentSource that doesn't exists anymore as a private string/object property but is always returned from the getFragmentSource function.
We still have a default value object otherwise we need to declare a constructor per instance.
Probably final typings will force us to add a constructor and in that case the concept of defaults for filters can be removed.
For now is at least terser because focus all the parameter logic in the base class.
The duality of BaseFilter or BaseFilter is not needed anymore and has been removed ( was introduced for the migration )
Removed the duality between diff and difference for BlendColor.
diff
was introduced 9 years ago, but the correct name isdifference
that was paired with it later. Was never deprecated