-
-
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): Converting the filtering backend to ts + es6 #8403
Conversation
Build Stats
|
i m adding base_filter class to this PR, so i don't have to wait reviews |
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.
Looks good
I saw what you meant about disposing and the types, I prefer your solution than any I can think of that just add complication where it isn't wanted nor needed
src/filters/2d_backend.class.ts
Outdated
dispose: noop, | ||
clearWebGLCaches: noop, | ||
constructor() { | ||
return this; |
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.
??
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 know... let me remove it, i don't remember
src/filters/webgl_backend.class.ts
Outdated
|
||
constructor({ tileSize } = {}) { | ||
constructor({ tileSize = config.textureSize } = {}) { | ||
if (tileSize) { |
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.
redundant check, always true, right?
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
@@ -65,10 +68,11 @@ class WebglFilterBackend { | |||
|
|||
/** | |||
* Pick a method to copy data from GL context to 2d canvas. In some browsers using | |||
* drawImage should be faster, but is also bugged for a small combination of old hardware |
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.
* drawImage should be faster, but is also bugged for a small combination of old hardware | |
* drawImage should be faster, but is also buggy for a small combination of old hardware |
* @param {WebGLRenderingContext} sourceContext The WebGL context to copy from. | ||
* @param {Object} pipelineState The 2D target canvas to copy on to. | ||
*/ | ||
copyGLTo2D(gl: WebGLRenderingContext, pipelineState: TWebGLPipelineState) { |
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 method can be static
I saw the code calls it with a call directive: copyGLTo2D.call(context, ...params)
but there is no need since it is context less
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.
Ahh I saw the assignment of copyGLTo2DPutImageData
that uses context
Why are those methods standalone and copyGLTo2D
is a class method?
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.
Perhaps create a dedicated lass for these methods, passing the context to the constructor?
Instead of using call and casting types, perf-wise is identical I believe
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.
yeah this method swap here isn't a good idea, but for now i m not going to touch it
@@ -526,7 +527,7 @@ import { FabricObject } from './fabricObject.class'; | |||
this._lastScaleY = 1; | |||
} | |||
if (!fabric.filterBackend) { | |||
fabric.filterBackend = fabric.initFilterBackend(); | |||
fabric.filterBackend = initFilterBackend(); |
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.
tree shaking should relief us from this task, right?
we can declare it in the file so if it is imported it is included and that's it... as the rest of fabric
well I guess for the cdn maybe....
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.
mmm i think there are cases in which you want to swap the instance.
It was happened here, but probably a setFilterBackend method is all we need in case you want to switch from 2d to webgl on request
how did i add 5k of code? |
i really don't like the 4.553 extra minified bytes whyyyy |
Just to brag a bit, this is around 4800 bytes of code
And is the totality of Canvas2dFilterBackend, WebglFilterBackend initFilterBackend and webglprobe.
And this is all the code we touched in this PR, so what, it doubled in size? |
base filter before PR 4161 bytes ( 450 less )
The rest was 5860
|
ok i got really crazy on this... if you just make a ls -l you will notice that:
while this PR
So is 1k smaller and not 4.5k bigger. |
In some ci PR I fixed the false reporting. It was caused due to changes in the index file that were accounted for in the master stats wrongly |
Motivation
Continue conversion of fabricJS to TS.
Move the filtering backend and the base filter to TS.
No functionality changes, just Types + modern js.
Description
Changes
Gist
In Action