Skip to content

fix(): gradient transform + rm workarounds#9359

Draft
ShaMan123 wants to merge 14 commits into
masterfrom
fix-gradient
Draft

fix(): gradient transform + rm workarounds#9359
ShaMan123 wants to merge 14 commits into
masterfrom
fix-gradient

Conversation

@ShaMan123

@ShaMan123 ShaMan123 commented Sep 20, 2023

Copy link
Copy Markdown
Contributor

Motivation

#9355

I was invetigating the bug and saw that the cause was the _applyPatternGradientTransformText
When I wrote #8263 I tried to fix this thing with a math solution and now I managed to.

Description

Previously in order to apply transform on a pattern/gradient fabric would create a canvas, transform it acoordingly and render a rect.
Then it would use the canvas to create a pattern and set that to the rendering context as fillStyle/strokeStyle.

  • CanvasPattern supports natively setTransform so I used that instead
  • gradientTransform is supported only for linear gradients (don't know why but I am fine with it because then my solution is valid) so I applied the transform to the coords
  • offsetX/Y are a transform so I applied that also when setting the trasform on the pattern/gradient

This is a very good change.
It removes a workaround that was complex, had bugs and was mentioned as a perf hit

Changes

Moved all pattern/gradient transform logic to toLive
BREAKING:

  • toLive(ctx) => toLive(ctx, target)`
  • rm _applyPatternGradientTransformText, _applyPatternGradientTransform

Gist

In Action

@github-actions

github-actions Bot commented Sep 20, 2023

Copy link
Copy Markdown
Contributor

Build Stats

file / KB (diff) bundled minified
fabric 910.093 (-5.584) 303.717 (-1.770)

c

cleaner

Update index.spec.ts

@ShaMan123 ShaMan123 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

progress

expect(
await createNodeSnapshot(
(canvas, fabric) => render(canvas, fabric, { type }),
{ width: 1000, height: 400 }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Something is buugy with canvas.setDimensions on node
I tried calling it in the render method but it doesn't work

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

moved canas init to the common method

@ShaMan123
ShaMan123 removed the request for review from asturur September 20, 2023 08:06
@ShaMan123
ShaMan123 marked this pull request as draft September 20, 2023 08:07
@ShaMan123

Copy link
Copy Markdown
Contributor Author

radial gradients must use a pattern :(

@asturur

asturur commented Sep 20, 2023

Copy link
Copy Markdown
Member

radial gradients don't support transform?

@ShaMan123

Copy link
Copy Markdown
Contributor Author

not natively

@ShaMan123

Copy link
Copy Markdown
Contributor Author

all gradients do not but I have found a math solution for the linear gradient transform i think

@ShaMan123

Copy link
Copy Markdown
Contributor Author

Correct me if I am wrong but it seems (radial) gradient needs context isolation to be transformed or else if transforming the context text for example will be transformed as well.
So, I am not sure fabric is ready for me to propose my solution.
And this is why a pattern was used.
In general I think we should pass a context in rendering and supply a canvas for context isolation operations

@asturur

asturur commented Sep 25, 2023

Copy link
Copy Markdown
Member

for sure there is a math solution to transorm linear gradients because are just points in a line and we either squeeze the points or change the angle and it will look identical.

But for radial gradients when circle becomes ellipses no you need to transform those.
Radial gradients are not so popular, if we have to keep around the old code for radial gradients is a pain but at least we solve the slow path of the linear gradients that are more common.

@ShaMan123 ShaMan123 added the stale Issue marked as stale by the stale bot label May 7, 2024
@stale stale Bot removed the stale Issue marked as stale by the stale bot label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants