Add CSS Color 4 syntax support - integration with changes from maplibre-style

[kajkal]

Integration with changes from PR #94 from maplibre-style.

• update color-related integration tests
• add color-interpolation render tests

Launch Checklist

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.
• Link to related issues.
• Include before/after visuals or gifs if this PR includes visual changes.
• Write tests for all new functionality.
• Document any changes to public APIs.
• Post benchmark scores.
• Add an entry to CHANGELOG.md under the ## main section.

[HarelM]

@kajkal can you try and update the branch here and let me know if everything is working as expected in terms of updating the spec version?

[kajkal]

Updated, colors (parsing/interpolation) are as expected, all tests are passing - it seems fine to me 👍

[HarelM]

Any chance for a render test? So that this change could be visually seen?

[kajkal]

Oh ok, those exist, I completely missed them and was only looking at the results of jest 😩

[kajkal]

As some render tests proved, there are cases in which an already parsed instance of Color class is passed to Color.parse function, contrary to my assumptions.
I created PR #128 - blocker for this PR - needs new maplibre-style-spec release as 19.0.0 is without this fix.

I added 2 render tests for color interpolation and added the ability to change the default sensitivity to color differences.
The tests in the previous configuration checked colors with a approximation: rgb(46,215,46) vs rgb(0,255,0) => pass.
By changing the sensitivity for tests directly related to colors, they are now able to detect a small change in the value of even one of the sRGB channels: rgb(28,199,255) vs rgb(28,200,255) => fail.

[drwestco]

You can use the 'keyof typeof' syntax here instead of string literals, so it keeps working when new interpolatable types are added. If we run into more cases needing the interpolationType, we can then consider a change in the style spec repo.

…

On Sun, Apr 16, 2023, 2:19 PM Karol Leśniak ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/style/properties.ts <#2376 (comment)> : > @@ -483,9 +483,10 @@ export class DataConstantProperty<T> implements Property<T, T> { } interpolate(a: T, b: T, t: number): T { - const interp: any = interpolateFactory(this.specification.type as any); - if (interp) { - return interp(a, b, t); + const interpolationType = this.specification.type as 'number' | 'color' | 'array' | 'padding'; Yes, you are right. This change would now require a separate PR to maplibre-gl-style-spec because I read your comment too late, so I'm not sure if it's worth it for those 2 repetitions - what do you think? — Reply to this email directly, view it on GitHub <#2376 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFVM5SY75IHJBZLUYRC4YDXBRO6NANCNFSM6AAAAAAWZVR43Y> . You are receiving this because you commented.Message ID: ***@***.***>

[HarelM]

Seems like there's a render test that is failing. Note that I have recently added some instructions on how to get an image from the CI to get the test passing. There is a report that should be attached to the CI run, you can get it by going to the actions tab here in github, I believe.

[kajkal]

Thanks, I was not aware that this was possible!
I adjusted the threshold for my new color interpolation tests - It should be working now.

[HarelM]

Generally approved, you'll need to set it for ready for review and resolve conflicts to have it merged.