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

DRAFT WIP BREAKING: remove opacity from colorstops in live Gradient class #9622

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

Conversation

asturur
Copy link
Member

@asturur asturur commented Jan 21, 2024

Description

Another item that comes up in my flame graphs often is the gradien 'toLive' function.
Half of the time ( 0.13 of 0.25 in most samples ) is spent parsing a color string and outputting a new rgba value.

That is wasteful since the couple 'color' and 'opacity' on separate values is something that is inherited from SVG and doesn't have to bleed into fabric internals.

This PR propose to remove opacity from the gradient colorStop type, merging it in the color string itself.

This PR would be a no brainer if it wasn't that compatibility with old saved objects needs to be maintained and so a compatibility function needs to be created and maybe removed later down in next versions of fabric.

In Action

Copy link

codesandbox bot commented Jan 21, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link
Contributor

github-actions bot commented Jan 21, 2024

Build Stats

file / KB (diff) bundled minified
fabric 914.618 (-0.089) 305.423 (-0.094)

@ShaMan123
Copy link
Contributor

You can/should optimize the calls.
If nothing changed toLive should return a stored ref.
I agree with all the above

@ShaMan123
Copy link
Contributor

We can always expose a migration function for data and then devs can handle their fromObject instead of polluting fabric.

@asturur
Copy link
Member Author

asturur commented Jan 22, 2024

We can always expose a migration function for data and then devs can handle their fromObject instead of polluting fabric.

Yes i will make a migration function, called exactly migrationForBreakingChangeXYZ that then will get removed when grace time passes.

@asturur
Copy link
Member Author

asturur commented Jan 22, 2024

You can/should optimize the calls. If nothing changed toLive should return a stored ref. I agree with all the above

I do not know if i can create a toLive with ctx of value X and then reuse it on a different ctx somewhere else.

We can measure if that has a different impact once this part is cleaned up

@asturur asturur added this to the 6.0 milestone Mar 16, 2024
@asturur asturur self-assigned this Mar 16, 2024
@asturur
Copy link
Member Author

asturur commented Mar 16, 2024

I may draft a spec for this, evaluate issue and try to fit into 6.0.
I m writing specs today

this.colorStops.push({
offset: parseFloat(position),
color: color.toRgb(),
opacity: color.getAlpha(),
color: colorStops[position],
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe those can be sort correctly every time we add new.
Unsorted colorstops are often cause of strange bugs.
Canvas doesn't care much but SVGs or PDFs do.

@asturur
Copy link
Member Author

asturur commented Mar 28, 2024

I wanted to see if i could complete this for 6.0 but requires AT LEAST making the json importer updater, so too much work pushed in rush

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants