Repository navigation
Conversation
jsnajdr
added
[Type] Performance
Co-authored-by: simison <simison@git.wordpress.org> Co-authored-by: aduth <aduth@git.wordpress.org> Co-authored-by: fcoveram <fcoveram@git.wordpress.org>
7a1ac0c to
f31873d
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
🤖 PR meta 🤖📦 Bundle sizeSize Change: +960 B (+0.01%) Total Size: 8.01 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
|
There was a problem hiding this comment.
This is an interesting approach! Also loved the illustration in the PR description :)
This PR currently lowers an existing text contrast case below its required level. With primary #608010 on background #4f386e, foreground.interactive.neutral-weak now has 4.488:1 contrast against both neutral interactive backgrounds instead of at least 4.5:1.
And more in general:
- is it expected that so many computed colors change?
- are you seeing any notable performance improvements? I could not, at least on my machine
| return ( | ||
| crossings.find( ( chroma ) => | ||
| channels.every( ( [ c3, c2, c1, c0 ] ) => { | ||
| const value = | ||
| ( ( c3 * chroma + c2 ) * chroma + c1 ) * chroma + c0; | ||
| return ( | ||
| value >= -SURFACE_EPSILON && value <= 1 + SURFACE_EPSILON | ||
| ); | ||
| } ) | ||
| ) ?? 0 |
There was a problem hiding this comment.
This check can turn a valid color gray.
For example, maxInGamutChromaAtLH( 0.9, 301, sRGB ) returns zero chroma, which means gray, even though the valid limit is about 0.05757. We should somehow make the check tolerate rounding differences, and make sure to test for a more granular set of hues between closer to pure black / whilte?
The 
At this point the max chroma is 0.19, but it's very close to the H=264 point where the max chroma is 0.30. If you're searching for a point whose distance from the surface is AI originally pointed this out, and recommended to retune the
Good find, this is a bug in the solver, where the 
Note that the curve is very flat near zero, and there is a wide interval where the value is "almost zero". That reduces the numerical precison of calculating the root.
I plan to check it once more after the recent refactors in #82591 and #82545. I expect a performance improvement, but most importantly, the answer to "what is the max chroma at LH" should be much more precise now, without any approximation. |
This branch has not been deployed
3 participants
Alternative to #82591 that calculates
maxInGamutChromaAtLHby a much faster and exactly precise algorithm.The idea is:
My initial measurements show that this is slightly slower than the original with cache and
toGamutCSS. 467ms vs 505ms. But I'm not 100% sure yet. The advantage is that the results are exactly precise.The current implementation is an unchanged AI output (Cursor with Opus 4.6). There are characteristically cryptic comments, and I don't yet have a full human understaning of it. I'd like to clean it up before merging.