Broaden UIGraphicsRenderer Experiment to Everywhere #1433
Conversation
|
This diff is a pain to review, sorry. Split view makes it a lot easier. One day github's diff will learn to ignore whitespace – it's only Microsoft, after all. |
FWIW |
|
There's that old-duck wisdom |
|
LGTM overall |
It wasn't helpful in the first place. Could be that the options were wrong on some platform, or whatever, but everyone turned it off. |
| ASExperimentalDrawing = 1 << 11, // exp_drawing | ||
| ASExperimentalFixRangeController = 1 << 12, // exp_fix_range_controller | ||
| ASExperimentalOOMBackgroundDeallocDisable = 1 << 13, // exp_oom_bg_dealloc_disable | ||
| ASExperimentalTransactionOperationRetainCycle = 1 << 14, // exp_transaction_operation_retain_cycle |
There was a problem hiding this comment.
Should re-ordering these be done with a separate diff?
There was a problem hiding this comment.
This is not re-ordering, it's replacing and it's an essential part of this diff.
Source/ASTextNode.mm
Outdated
| @@ -538,64 +538,30 @@ + (UIImage *)displayWithParameters:(id<NSObject>)parameters isCancelled:(NS_NOES | |||
| { | |||
| ASTextNodeDrawParameter *drawParameter = (ASTextNodeDrawParameter *)parameters; | |||
|
|
|||
| if (drawParameter->_bounds.size.width <= 0 || drawParameter->_bounds.size.height <= 0) { | |||
| if (CGRectIsEmpty(drawParameter->_bounds)) { | |||
There was a problem hiding this comment.
Should this be done with a separate diff?
There was a problem hiding this comment.
I don't know. Should it?
Source/ASTextNode.mm
Outdated
| if (!renderedWithGraphicsRenderer) { | ||
| UIGraphicsBeginImageContextWithOptions(CGSizeMake(drawParameter->_bounds.size.width, drawParameter->_bounds.size.height), drawParameter->_opaque, drawParameter->_contentScale); | ||
|
|
||
| return ASGraphicsCreateImageWithOptions(drawParameter->_bounds.size, drawParameter->_opaque, drawParameter->_contentScale, nil, ^{ |
There was a problem hiding this comment.
@garrettmoon is still in the midst of rolling this experiment out. Please let him review before landing if possible.
There was a problem hiding this comment.
Yep, won't land without Garrett's take. I can break this apart into "TextDrawing" and "NonTextDrawing" if that is useful.
|
Note: don't land until @garrettmoon weighs in. Garrett, it's not a problem to leave your experiment and change this diff to add a separate one for the rest of the drawing. Let me know which way you want to go. |
|
🚫 CI failed with log |
|
I'd like to get a bit more data from the text experiment before we merge this. Thank you @Adlai-Holler for putting this up! |
…where, when available
Adlai-Holler
force-pushed
the
AHImageContexts
branch
from
ca3834c
to
b325901
Compare
|
OK let's leave your experiment intact. Reopening as a separate experiment. |
Generated by 🚫 Danger |
|
🚫 CI failed with log |
This gets rid of the ASExperimentalGraphicsContext experiment that was a dud, and replaces ASGraphicsContexts.h with a wrapper. The wrapper calls into either UIGraphicsBeginImageContext or UIGraphicsImageRenderer to build an image.
These graphics renderers seem pretty awesome. https://gist.github.com/NSProgrammer/38f1b4f9b29c3077af3f39a7e98092b1
Want multiple core-team reviews on this since it touches such core code. It's pretty straightforward though. The one case where it wasn't straightforward (
__didDisplay…) I just left with UIGraphics.