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

fix(): gradient transform + rm workarounds #9359

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Sep 20, 2023

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
Copy link
Contributor

github-actions bot commented Sep 20, 2023

Build Stats

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

c

cleaner

Update index.spec.ts
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

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
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
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
Contributor Author

radial gradients must use a pattern :(

@asturur
Copy link
Member

asturur commented Sep 20, 2023

radial gradients don't support transform?

@ShaMan123
Copy link
Contributor Author

not natively

@ShaMan123
Copy link
Contributor Author

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

@ShaMan123
Copy link
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
Copy link
Member

asturur commented Sep 25, 2023

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