-
Notifications
You must be signed in to change notification settings - Fork 546
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
More Span<T> and ReadOnlySpan<T> #2669
base: main
Are you sure you want to change the base?
Conversation
I'll make some tests later. Feedback on the null-terminated LPArray/LPStr situation would be appreciated! |
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.
Is there a specific reason public Result Shape(string text, float xOffset, float yOffset, SKFont font)
uses buffer.AddUtf8
instead of buffer.AddUtf16
?
This comment was marked as outdated.
This comment was marked as outdated.
This would be fantastic to include especially for rendering partial arrays like on the |
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.
Sorry folks for the extended delay in getting to this one. I had a quick check and I think it is mostly good.
However, this is going to break ABI because the parameters have changed and there will be a bunch of missing member exceptions. Is it possible to instead add overloads and make the old array methods call those overloads.
Could you rebase and rather add overloads.
I also added a few extra comments to things that I saw maybe should not be changed in this PR.
Thanks again for this effort!
public ushort[] GetGlyphs (ReadOnlySpan<char> text) | ||
{ | ||
using var font = ToFont (); | ||
return font.GetGlyphs (text); | ||
} | ||
public ushort[] GetGlyphs (ReadOnlySpan<char> text) => | ||
GetFont ().GetGlyphs (text); |
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 see this changes ToFont
into GetFont
, and I am not sure why I did this in the beginning. Maybe just use ToFont
for now and then if I was wrong, then we can fix it 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.
Seeing as the surrounding methods (including some overloads) use GetFont
instead of ToFont
and because SKFont.GetGlyphs
is a pure method, I think it is safe to use ToFont
in this situation. If I'm wrong though, I can move the change to a new PR for later review.
Thanks for the response! I'll get working on the requested changes.
I presume this means this PR will supersede #2617?
Yeah, I'm not sure what I was thinking with some of the changes either. It probably had something to do with the changes being authored at 4am 😅 |
…oads are still useful. Some null checks have been removed as they are immediately followed by minimum length checks, which effectively accomplish the same goal.
The length checks immediately following the null checks accomplish the same goal.
…apedText to use ReadOnlySpan<char>
In Because this is a breaking change, I do not want to make the call in this PR, but I would like to start a discussion on whether it can be done in order to maintain output parity between the string and span overloads. |
Description of Change
I have add Span overloads to many methods that previously only accepted Arrays, IntPtrs, or Strings.
Some methods could not be given span overloads because the Array/Strings must be marshaled as null-terminated types. This could be solved by copying the contents of the span into a rented array and filling the rest of the array with
null
, however I wanted to discuss this change before making it.The next changes aren't strictly related to converting to spans, but I figured they were related enough to include them here. If not, I can move them to a new PR.
I changed a few
return new T[0]
toreturn Array.Empty<T> ()
to reduce pointless allocations. As far as I can tell, this does not change the behaviour of dereferencing a pointer to the array.I also changed the algorithm of
implicit operator SKRuntimeEffectUniform (float[][] value)
to make allocations linearly associated with the length ofvalue
rather than using a variable length list.Lastly, I believe I fixed 4 memory leaks in
SKCanvas.DrawVertices
, swapped out 3SKTypeFace.ToFont
calls toSKTypeFace.GetFont
inSKTypeFace.GetGlyphs
, and fixed a potential(?) nullptr dereference inSKPath.GetPoints
. If any changed behaviour occurs in those methods as a result, that might have been my fault.Bugs Fixed
Span<T>
orReadOnlySpan<T>
#2616API Changes
new T[0]
forArray.Empty<T> ()
Behavioral Changes
None.
Required skia PR
None.
PR Checklist