-
Notifications
You must be signed in to change notification settings - Fork 756
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
fix: Use Rgba8888 for Skia/macOS/metal #18231
Conversation
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18231/index.html |
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18231/index.html |
The build 140214 found UI Test snapshots differences: Details
|
We would need to test these changes on a Mac intel to decide if the changes are ok to backport to 5.4 |
@spouliot @agneszitte Thanks for fixing this issue, I was not aware. Regarding the backport, does it mean the fix will be available in a future big update? 5.4.5 was released yesterday. I don't know the merge structure. |
@ArchieCoder We have yet to validate the change, but you can validate that it does what you need using 5.5-dev builds. |
@spouliot I tested with 5.5.0-dev.66 and I have the same effect as before (screenshot shows the app and the global.json) |
Unexpected. I'll check if Just in case, did you do a clean after updating ? are you using a arm64 or Intel Mac ? thanks! |
So looking into Make sure a |
@spouliot How do I clean or do a restore? I don't even see this in VS Code. I deleted the folders obj/bin. It had no effect. I have arm64. |
@ArchieCoder usually
That's done automatically by our VS Code extension - so that should not be an issue.
Thanks, I'll be checking stuff on my side... [1] but |
@ArchieCoder I can duplicate with the same packages... 😞 weird |
@spouliot sad for the bug but happy that you see the same thing |
@spouliot that means that more fixes will be needed for ScottPlot/ScottPlot#4257 ? |
@agneszitte yes |
Ok, so previous, similar issues have tinted my glasses. Unlike them this happens on both software and hardware(metal) rendering - which otherwise are working normally. Still the error does not seems to come from ScottPlot. Took m a while to convince myself but I can now duplicate the issue with other samples that, like ScottPlot, also use Note: I can't be 100% sure but I probably executed the Catalyst (instead of desktop) version of the app when testing my previous fix :-( |
Could be related to mono/SkiaSharp#2918 |
@spouliot Just to be sure, could you actually be hitting the SkiaSharp bug, then your fix will break when run against fixed SkiaSharp version? IIRC, the version we run on is 2.88.7 which doesn't have the fix, I think? |
We need to align our usages everywhere, really. Otherwise it's getting too problematic |
I'm running with 2.88.8 but I'll double check if the fix is present (or not).
Yes. There's some platforms that only support some format(s), but most do support whatever is requested. |
Per release notes, it seems the fix is included in 2.88.9 p1.1 https://github.com/mono/SkiaSharp/releases/tag/v2.88.9-preview.1.1 |
So |
GitHub Issue (If applicable): closes #
ScottPlot/ScottPlot#4257
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Metal-based rendering was using
Bgra888
and copyingRgba888
memory surface into it is swapping theR
andB
channels.What is the new behavior?
Metal-based rendering was using
Rgba888
PR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Run
results.Other information
Internal Issue (If applicable):