Skip to content

feat(Gradient): BREAKING: Remove opacity from colorstops in live Gradient class #9622

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

Merged
merged 31 commits into from
Apr 13, 2025

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 916.932 (-0.179) 301.785 (-0.135)

@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

@asturur asturur marked this pull request as ready for review April 4, 2025 20:56
@asturur asturur changed the title DRAFT WIP BREAKING: remove opacity from colorstops in live Gradient class feat(Gradient): BREAKING: Remove opacity from colorstops in live Gradient class Apr 4, 2025
@fabricjs fabricjs deleted a comment from github-actions bot Apr 4, 2025
@fabricjs fabricjs deleted a comment from github-actions bot Apr 4, 2025
Copy link
Contributor

github-actions bot commented Apr 7, 2025

Coverage after merging colorstops-in-render into master will be

72.11%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
fabric.ts100%100%100%100%
index.node.ts11.76%100%16.67%7.41%11, 15, 19, 21–31, 39–49
index.ts0%0%0%0%1, 1
publish-next.js0%0%0%0%1, 1, 11, 13, 15, 3, 5, 7, 9
publish.js0%0%0%0%1, 1, 10, 12, 14–15, 17, 3–4, 6–9
extensions
   index.ts0%0%0%0%1, 1, 11, 6
extensions/aligning_guidelines
   constant.ts28.57%100%100%0%10, 3, 5, 7, 9
   index.ts0%0%0%0%1, 1, 10, 100–109, 11, 110–119, 12, 120–129, 13, 130–139, 14, 140–149, 15, 150–159, 16, 160–166, 17–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–79, 8, 80–89, 9, 90–99
   typedefs.ts100%100%100%100%
extensions/aligning_guidelines/util
   basic.spec.ts0%0%0%0%1, 1, 10–12, 14–19, 2, 20–29, 3, 30–31, 5–9
   basic.ts10.53%100%100%0%10–20, 3–5, 7–9
   collect-line.ts0%0%0%0%1, 1, 10, 100–109, 11, 110–119, 12, 120–129, 13, 130–139, 14, 140–149, 15, 150–159, 16, 160–169, 17, 170–172, 18–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–79, 8, 80–89, 9, 90–99
   collect-point.ts2.82%100%100%0%12–13, 15–19, 2, 20–23, 25–29, 3, 30–40, 42–50, 52–67, 69–83
   draw.ts0%0%0%0%1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–74, 8–9
   get-objects-by-target.ts0%0%0%0%1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–45, 5–9
extensions/data_updaters/gradient
   index.ts15.38%100%100%0%11–12, 18, 20, 22–28
extensions/data_updaters/origins
   index.spec.ts0%0%0%0%1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–79, 8, 80–89, 9, 90–98
   index.ts0%0%0%0%1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70, 8–9
src
   ClassRegistry.ts94.64%78.57%100%100%
   Collection.ts78.26%52.94%90.32%83.63%112, 130, 153, 155–157, 159, 181, 197, 215, 217, 241, 243, 252, 265, 270, 279, 286–287, 302, 309–310, 329, 338–344, 346–347, 350
   CommonMethods.ts89.80%76.92%100%93.33%50, 52
   EventTypeDefs.ts100%100%100%100%
   Intersection.ts81.65%50%100%95.83%184–188, 190, 237, 239, 289, 297, 297
   Observable.ts77.54%65.79%93.75%79.76%144, 153, 156, 168–170, 33, 50, 68–70, 76, 80, 84–85, 87, 89–91
   Point.ts90.04%69.49%100%96.38%48–50, 71–72
   Shadow.ts81.88%75%100%81.82%140, 145–150, 159, 196, 199, 206, 223–230, 234–235
   cache.ts92.16%66.67%100%97.37%57, 59
   config.ts71.91%50%66.67%80%136, 138–141, 143, 143, 143–147, 151, 156–158
   constants.ts100%100%100%100%
   typedefs.ts100%100%100%100%
src/LayoutManager
   ActiveSelectionLayoutManager.ts92.59%76.92%85.71%100%
   LayoutManager.ts90.06%68.42%84.62%97.83%279–280, 344, 354–355, 63
   constants.ts100%100%100%100%
   index.ts100%100%100%100%
   types.ts100%100%100%100%
src/LayoutManager/LayoutStrategies
   ClipPathLayout.ts72.46%50%100%77.55%41–44, 46–48, 57, 64, 66–69
   FitContentLayout.ts100%100%100%100%
   FixedLayout.ts80%33.33%100%100%23, 23
   LayoutStrategy.ts88.12%67.86%100%95.45%53–54, 73–75
   utils.ts75.86%43.75%100%87.80%39–44, 47
src/Pattern
   Pattern.ts50%82.35%83.33%36.36%105, 107, 114, 118–119, 119–122, 130, 132, 134–140, 143, 153–164, 174, 176–181, 183–187, 190–199, 203–207, 209–210, 212–214, 216–218, 218–221, 33, 36–37
   index.ts100%100%100%100%
   types.ts100%100%100%100%
src/brushes
   BaseBrush.ts82.05%81.82%87.50%81.36%110, 115, 124, 135, 146, 155–158, 160, 99
   CircleBrush.ts27.17%28.57%22.22%27.63%100–106, 108, 108, 110–112, 114–116, 118–122, 13, 130–134, 136, 138–139, 35, 40, 42, 44, 51–55, 63–64, 67, 69, 77–79, 79, 79–83, 85–86, 92–93, 95, 97–99
   PatternBrush.ts69.81%100%100%60.98%25–31, 34, 36, 44, 54, 64–67, 69
   PencilBrush.ts72.12%78.38%100%67.95%116, 122–124, 129, 135, 139, 145, 147–149, 151–152, 152–155, 158, 164, 169, 176, 176, 189, 198, 205, 212, 215, 222, 224–231, 235, 238, 243, 246, 248, 255–256, 260–261, 266, 273, 278, 280, 285–287, 299–300, 68–69, 84–85
   SprayBrush.ts4.62%0%0%5.45

Copy link
Contributor

github-actions bot commented Apr 12, 2025

Build Stats

file / KB (diff) bundled minified
fabric 917.131 (+0.179) 307.398 (+5.614)

Copy link
Contributor

github-actions bot commented Apr 12, 2025


> [email protected] coverage:report:ci
> nyc report --reporter=text-summary


=============================== Coverage summary ===============================
Statements   : 70.53% ( 20317/28806 )
Branches     : 60% ( 3752/6253 )
Functions    : 87.92% ( 1588/1806 )
Lines        : 75.3% ( 15634/20760 )
================================================================================

@asturur asturur merged commit 580bd5d into master Apr 13, 2025
21 checks passed
@asturur asturur deleted the colorstops-in-render branch April 13, 2025 13:30
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