chore(TS): BREAKING move also defaults for filters away from prototype #8742

asturur · 2023-02-26T20:34:07Z

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 is difference that was paired with it later. Was never deprecated

github-actions · 2023-02-26T20:35:52Z

Build Stats

file / KB (diff)	bundled	minified
fabric	891.502 (-1.263)	304.527 (-1.771)

asturur · 2023-02-28T07:29:30Z

the fact that all the code i added saved 1.3kb leaves me a bit confused.
I definitely added more than removed, with all the getFragmentSource and imports files

asturur · 2023-02-28T07:43:04Z

Ok this is ready to go.
There are probably optimizations that can be done for main parameter ( but unless they save me another half kb i m not committing them )

asturur · 2023-02-28T07:55:35Z

This is ready to go i think.
And i think it completes the part of the default values that is necessary for 6.0
If i can get away from type from objects as i did for filter i ll remove also that, otherwise not.

asturur · 2023-02-28T07:56:50Z

oh no controls yet.
But controls can be solved cleanly with the same strategy.

ShaMan123 · 2023-02-28T08:16:17Z

I will review now.
I did type as you suggested #8714

ShaMan123

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 folder Shaders and move all of them there. I like that you took it out and I like the idea of having them in a Shaders folder. They are consts so seems right to me.
moving fromObject to base class with new 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

ShaMan123 · 2023-02-28T08:19:35Z

src/filters/BlendColor.ts

 import { T2DPipelineState, TWebGLUniformLocationMap } from './typedefs';
 import { classRegistry } from '../ClassRegistry';
+import { blendColorFragmentSource } from './blendColor.shaders';
+
+type TBlendMode =


I think we should export most types in general

ShaMan123 · 2023-02-28T08:26:22Z

src/filters/Blur.ts

+
+export const blurDefaultValues: Partial<TClassProperties<Blur>> = {
+  blur: 0,
+  mainParameter: 'blur',


shouldn't this be static as well?

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.

(i already tried 3 times to remove it entirely or reduce its appareances)

ShaMan123 · 2023-02-28T08:33:31Z

src/filters/ColorMatrixFilters.ts

 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;
 }


I would make this not a mixin but a subclass
See https://github.com/fabricjs/fabric.js/blob/08e9431b6c19f763216f9061d7cec25774c58961/src/filters/ColorMatrixFilters.ts

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.

but this is not a mixin. this is a subclass generated with a function.

I meant dynamic classes, sorry

ShaMan123 · 2023-02-28T08:37:36Z

src/filters/Resize.ts

+  fragmentSourceTOP: `
+    precision highp float;
+    uniform sampler2D uTexture;
+    uniform vec2 uDelta;
+    varying vec2 vTexCoord;
+  `,


for consistency should be moved to a standalone file?

this resize one is too messy. but shouldn't be a property either, i will move this

ShaMan123 · 2023-02-28T08:37:48Z

src/filters/Resize.ts

+  `,
+};
+
+type TResizeType = 'bilinear' | 'hermite' | 'sliceHack' | 'lanczos';


src/filters/BaseFilter.ts

src/filters/BlendImage.ts

ShaMan123 · 2023-02-28T09:25:51Z

moving fromObject to base class with new this will save a lot of lines, there are only 2-4 filters that override it

when you do this make sure to spread the object and remove type from it so it won't override the getter (I am guessing that is happening currently)

asturur · 2023-02-28T10:04:33Z

moving fromObject to base class with new this will save a lot of lines, there are only 2-4 filters that override it

when you do this make sure to spread the object and remove type from it so it won't override the getter (I am guessing that is happening currently)

No is not happening because the constructor is avoing getting type in.
Right i forgot about new this(), and i played around with gettin the class from class registry and then i didn't like it.
i ll do this now, but not touching anything else for now.

asturur · 2023-02-28T10:07:39Z

naming: can we keep camelCase or UpperFirst convesion? colorMatrix.shaders => ColorMatrixShaders, or perhaps create a folder Shaders and move all of them there. I like that you took it out and I like the idea of having them in a Shaders folder. They are consts so seems right to me.

i can move them in a folder yes, that will make the .shader. go away

asturur · 2023-02-28T10:19:24Z

extract the rest of WebGL c code (is it c?) from filter files

is shader language, not sure the exact definition.
The syntax is C like but the rules are different.
Also the assumptions are different

asturur · 2023-02-28T11:02:57Z

There are additional things that can be done here:

resize filter can have the shader code extracted ( followup )
some optimizations on how we do sendUniformData and getUniformLocations so that they can be defined as a map of a function rather than methods (helps abstracting writing a filter) ( follow up)
the highp/medium/low precision code can be made common and injected in all shaders

asturur added 29 commits February 21, 2023 09:56

moved state properties to static

50273fb

default values

280a3a4

maybe this

9ebafef

some mild progress

24bd7cc

more progress

a4e0550

shapes done

41d8730

image

76b0e7b

Merge branch 'master' into default-values-3

2707ced

fix broken merge

38a7c0b

revert filters

f46a009

more test fixups

787e1a4

fixes strokeFillOpacity

7b913ea

prettier and changelog

5b21ee1

removed const from string array

0fde1fb

removed const from string array

5f1eef3

renamed get default

ea6c3e0

and of course prettier

1cf207f

removed debug statement

5bd8f01

Merge branch 'master' into default-values-3

9a8a084

fix prettier

60fc8cc

renamed static strokeBboxaffecting...

c5a6870

remove-use-of-registry

3a24403

shorten

edf1e07

prettier

2503352

static-version

7c28cfd

working on filters

b9b9d92

working on filters

107d089

merge master

b424b7a

more filters work

8df5012

asturur added 2 commits February 28, 2023 01:57

more work

6193eae

apply2d

e5195d8

prettier

764b6ee

asturur changed the title ~~WIP: move also defaults for filters away from prototype~~ chore(TS): BREAKING move also defaults for filters away from prototype Feb 28, 2023

asturur added 2 commits February 28, 2023 08:36

removed diff

41d24d5

added comment

78d2336

asturur requested a review from ShaMan123 February 28, 2023 07:53

forget bolierplate code

49aab7e

ShaMan123 suggested changes Feb 28, 2023

View reviewed changes

ShaMan123 added a commit that referenced this pull request Feb 28, 2023

checkout filters in favor of #8742

7364291

asturur added 2 commits February 28, 2023 11:13

less code maybe

af99e27

move shaders

5d3a938

asturur added 4 commits February 28, 2023 11:32

move shaders

2a82a83

move shaders

0e37f24

move shaders

dec1d27

a space

20a1935

asturur added 2 commits February 28, 2023 12:06

extracted more shaders

19f28c5

prettier

8af551b

asturur merged commit 0a37035 into master Feb 28, 2023

asturur deleted the defaults-for-filters branch February 28, 2023 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(TS): BREAKING move also defaults for filters away from prototype #8742

chore(TS): BREAKING move also defaults for filters away from prototype #8742

asturur commented Feb 26, 2023 •

edited

Loading

github-actions bot commented Feb 26, 2023 •

edited

Loading

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

ShaMan123 commented Feb 28, 2023

ShaMan123 left a comment

ShaMan123 Feb 28, 2023

ShaMan123 Feb 28, 2023

asturur Feb 28, 2023

asturur Feb 28, 2023

ShaMan123 Feb 28, 2023

ShaMan123 Feb 28, 2023 •

edited

Loading

asturur Feb 28, 2023

ShaMan123 Feb 28, 2023

ShaMan123 Feb 28, 2023

asturur Feb 28, 2023

ShaMan123 Feb 28, 2023

ShaMan123 commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

chore(TS): BREAKING move also defaults for filters away from prototype #8742

chore(TS): BREAKING move also defaults for filters away from prototype #8742

Conversation

asturur commented Feb 26, 2023 • edited Loading

Motivation

github-actions bot commented Feb 26, 2023 • edited Loading

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

ShaMan123 commented Feb 28, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 28, 2023

asturur commented Feb 26, 2023 •

edited

Loading

github-actions bot commented Feb 26, 2023 •

edited

Loading

ShaMan123 Feb 28, 2023 •

edited

Loading