forked from facebook/react-native
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Support for Wide Gamut (DisplayP3) Colors to React Native #1
Open
ryanlntn
wants to merge
40
commits into
main
Choose a base branch
from
feat/wide-gamut-color
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
134b675
update normalizeColor and processColor to handle color()
ryanlntn 396d05e
initial DisplayP3 support for iOS
ryanlntn f4539c6
fix testIDs
ryanlntn ee6d760
update js to use enum for color space
ryanlntn 12e3c59
add global flag and reuse createColorFrom
ryanlntn 88b16da
add static defaultColorSpace to ColorComponents
ryanlntn 284653c
add colorSpaceFromString remove nested ternary
ryanlntn 51414ea
fix RCTConversions and graphicsConversions
ryanlntn 4c81a65
missed a cast
ryanlntn c5eab07
fix ColorComponents defaultColorSpace by providing getter
ryanlntn c9bddc8
update graphicsConversions to use getDefaultColorSpace
ryanlntn 53090da
fix animated interpolation without native driver
ryanlntn a36c9fc
handle color space in AnimatedColor
ryanlntn 7c5eeb9
mostly fix animation with native driver
ryanlntn 4b6a0e8
temp include example animation
ryanlntn 53aba9b
split out ColorComponents implementation to fix Android build
ryanlntn 9f30b94
Revert "split out ColorComponents implementation to fix Android build"
ryanlntn f303df1
fix RCTGetDefaultColorSpace and RCTSetDefaultColorSpace
ryanlntn f975b6b
refactor away from RCTColorFromComponents in RCTColorAnimatedNode
ryanlntn 1b41b02
refactor RCTInterpolateColorInRange
ryanlntn 6a99355
Reapply "split out ColorComponents implementation to fix Android build"
ryanlntn 2083fa0
initial Android handling of color function values (always sRGB for now)
ryanlntn f396fc3
fix crash in ColorAnimatedNode
ryanlntn e4b0949
fix color interpolation with native driver by reprocessing as srgb
ryanlntn 506d1ab
set ReactActivity window color mode to COLOR_MODE_WIDE_COLOR_GAMUT
ryanlntn 96ade88
get p3 backgrounds working
ryanlntn d3895f9
get display-p3 borders working in Android
ryanlntn a8d00a4
fix issues with bad defaults
ryanlntn e2884ad
update remaining color annotated props to long
ryanlntn f21a2a5
update MapBuffer to support long values
ryanlntn f0e1f69
change toAndroidRepr return type to int64_t
ryanlntn d65366e
update toAndroidRepr to support DisplayP3
ryanlntn 5352b6d
update TextAttributesProps to support long colors
ryanlntn b5431ee
update ReactForegroundColorSpan so P3 text works
ryanlntn 366001c
fix issues with basic FlatList example
ryanlntn 6344460
update foreground and background color spans
ryanlntn e8cee4d
pass longs through scroll view managers to scroll views
ryanlntn d07c577
update endFillColor to support long colors
ryanlntn c6f8d99
update switch
ryanlntn b072f06
clean up ReactViewBackgroundDrawable
ryanlntn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cipolleschi I'm still struggling with how to handle the unclamped values here. If I understand Apple's docs correctly we'd need to pass those to colorWithRed:green:blue:alpha: so we'd want to stay in extended srgb color space. But this doesn't appear to work.
Also I'm not sure I understand why I can't just use UIColor. When I try to return the UIColor directly here or from RCTColorAnimatedNode I don't get the color to come through at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I completely understand your questions, but:
This is true if the source space is sRGB.
If we are in displayP3, values that are < 0 and > 1 will not be properly represented. 🤔
I think we need to create the final
UIColor
by usingcolorWithDisplayP3Red:green:blue:alpha:
.If we change the return type from
int32_t
to something else, we have to update the callsites to understand the new return type, no?I'm not super familiar with this part of the codebase, I need to dig deeper here.
If this is the only blocker, I think we can move forward for the time being and consider this a known-issue: for the first implementation interpolateColor don't work with displayP3.
Even with this limitation, this would already be a massive improvement! I think that animating colors is not something every app actually require, so it is an edge case that it is ok to point out as "not working" for version 1 of the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
colorWithDisplayP3Red:green:blue:alpha:
gives me the same behavior as before including the color space and I believe it's because per Apple:and
Meanwhile with
colorWithRed:green:blue:alpha:
So I believe if we have unclamped values we may actually need to use
colorWithRed:green:blue:alpha:
to have them interpreted correctly. But then again I couldn't get it to work completely either way.As far as returning UIColor directly goes I updated the call sites for
RCTInterpolateColorInRange
andRCTColorAnimatedNode
but I was having trouble following what happens to the values afterperformUpdate
is called. Essentially I'd like to know how the animated values ultimately get applied back to the original views.Makes sense. I'll move on to Android today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To explore how the colors change based on the rgb values, I created this small project: https://github.com/cipolleschi/P3Colors.
feel free to clone it and play with it.
The top-left view is sRGB
The to-right view is displayP3
The bottom-left view is sRGB when we passed in the rgb values obtained by
getRed:green:blue:alpha
method on the displayP3 color.The bottom-right view is displayP3 when we passed in the rgb values obtained by
getRed:green:blue:alpha
method on the displayP3 color.Use a retina display to look at it! 😄
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-10.at.15.49.28.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. It looks like my interpretation of Apple's docs is correct then since passing the DisplayP3 colors to
colorWithRed:green:blue:alpha:
gives the same color as the original DisplayP3 color while passing tocolorWithDisplayP3Red:green:blue:alpha:
is too red up until you hit values greater than 1 at which point it's the same.The behavior right now in
RCTInterpolateColorInRange
is even more bizarre then since it's currently cycling through yellow and blue as well. 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... it hink that the behavior of interpolate is wrong in the context of display P3 because the color space is non euclidean. So the usual interpolation formula (which are linear) would not work properly there... 😓
Unfortunately, I don't have a clear solution...
One idea we can try to:
red
in P3 tored
in sRGB)blue
in P3 toblue
in sRGB).In that way we minimize the weirdness paying the tradeoff that interpolation is in sRGB... but if that happens mainly in animation, they should be "fast" enough so that the user won't notice that they are not in displayP3 space...
What do you think?