Skip to content

Drawing.BitmapFromArgbs: Handle Rgba colortype#4648

Merged
swharden merged 4 commits intoScottPlot:mainfrom
bclehmann:fix/heatmap-rgba
Jan 4, 2025
Merged

Drawing.BitmapFromArgbs: Handle Rgba colortype#4648
swharden merged 4 commits intoScottPlot:mainfrom
bclehmann:fix/heatmap-rgba

Conversation

@bclehmann
Copy link
Member

This fixes #4647, which is most easily replicated by launching the Blazor Web Assembly demo.

As I said in #4647 (comment) I might want to get rid of the bit swizzling and just add a Color.Abgr (the endian-swapped version of Rgba) property and deprecate BitmapFromArgbs in place of a version that takes a Color struct. I haven't done so in this PR because I figured I'd have more and better thoughts on a day that isn't new years 🙃.

Note that ARGB <-> BGRA and ABGR <-> RGBA due to endianness, Microsoft
prefers the first notation but Skia prefers the second.
@ssharks
Copy link

ssharks commented Jan 2, 2025

Thanks for your quick response, I found an similar bug report: mono/SkiaSharp#1492 (comment).
This advices to check to the SKImageInfo.PlatformColorType and depending on its outcome use the SKSwizzle function to do the swizzeling.

@ssharks
Copy link

ssharks commented Jan 2, 2025

What is interesting is that the coloring of the heatmap is OK when using it in a Jupiter environment. So I'm a little worried that this fix might break this option.

The code I used in .Net Interactive:

#r "nuget:ScottPlot"
using Microsoft.DotNet.Interactive.Formatting;
Formatter.Register(typeof(ScottPlot.Plot), (p, w) => 
    w.Write(((ScottPlot.Plot)p).GetPngHtml(800, 600)), HtmlFormatter.MimeType);

using ScottPlot;

ScottPlot.Plot myPlot = new();

// create sample data
double[] dataX = { 1, 2, 3, 4, 5 };
double[] dataY = { 1, 4, 9, 16, 25 };

double[,] data = new double[5,10];
for (var x=0; x<5; x++) {
    for (var y=0; y<10; y++) {
        data[x,y] = x+y;
    }
}
var hm = myPlot.Add.Heatmap(data);
hm.Rectangle = new CoordinateRect(0.5, 5.5, 0, 25);

// add a scatter plot to the plot
myPlot.Add.Scatter(dataX, dataY);

//myPlot.SavePng("demo.png", 400, 300);
myPlot

@bclehmann
Copy link
Member Author

What is interesting is that the coloring of the heatmap is OK when using it in a Jupiter environment. So I'm a little worried that this fix might break this option.

Good instincts, it does indeed break it, though it seems inconsistent after making changes, there may be some sort of caching in polyglot notebooks that's causing trouble with local packages if those packages are updated. I suspect I'll have better luck making a CLI app to debug this more reliably.

I suspect the issue could be that we're trying to intuit the colour type from the platform (by using the default colour type for an SKBitmap) but the colour type is fixed to Rgba8888 for GetPngHtml without GetBitmapFromArgbs knowing. I must be missing something though, since I'd only expect that issue if it were fixed to Bgra8888. I think a solution would be to pass in an optional colour type to BitmapFromArgbs.

In any case, this does seem like a more fragile solution than I'd expected, and that we do in many places fix the color type to Rgba, and never to Bgra. It makes me think that the solution should be to just create an SKBitmap which is explicitly designated as Bgra8888 and doesn't try to bit-swizzle at all. Which is probably what I would've done if I hadn't written this code on new years. My hope is that Skia should be smart enough to handle that when it's blitted onto a surface of a different colour type.

@bclehmann bclehmann marked this pull request as draft January 3, 2025 04:02
@swharden
Copy link
Member

swharden commented Jan 4, 2025

Hi @bclehmann, I see this is still marked as a draft PR. Do you want me to merge it in for the next release (about 24 hours from now), or give you more time to look into this?

I'm in favor of splitting this into two functions and hiding the call behind a feature flag (defaulting to the present behavior) if you are unsure how to proceed at this time.

Thanks for this PR and your input! It always makes me happy to see bit twiddling :)

Hope your 2025 is off to a great start!

@bclehmann bclehmann marked this pull request as ready for review January 4, 2025 20:57
@bclehmann
Copy link
Member Author

Hi @bclehmann, I see this is still marked as a draft PR. Do you want me to merge it in for the next release (about 24 hours from now), or give you more time to look into this?

It should be ready for review now, I've implemented the solution I should've from the start. I'm no longer able to test the behaviour in a .NET notebook, since it seems like my updates to my local package are no longer respected? Some weird caching problem I'm sure.

But the current solution should be much less fragile, frankly if this issue still shows up it would be because of a Skia bug (e.g. ignoring the SKColorType field). I would still welcome more testing, since it does freak me out that .NET notebook no longer responds to changes to the ScottPlot dll.

Thanks for this PR and your input! It always makes me happy to see bit twiddling :)

Alas the bit twiddling is now gone, sorry to disappoint.

Hope your 2025 is off to a great start!

So far I have no complaints, hope you're keeping well!

@ssharks
Copy link

ssharks commented Jan 4, 2025

The fix seems reasonable. I would like to validate it, are there any written instructions how to validate these changes that are not yet published using NuGet?

@bclehmann
Copy link
Member Author

bclehmann commented Jan 4, 2025

The fix seems reasonable. I would like to validate it, are there any written instructions how to validate these changes that are not yet published using NuGet?

There are instructions on building from source here: https://scottplot.net/faq/environment/ It looks like right now it's pretty simple, but there used to be more specific instructions for people building on older .NET SDKs (specifically ones that don't support the newest language features). You might also want to unload certain projects, either because they take a long time to build or because you don't have the prerequisites installed (the Maui projects are a good candidate, since they can take a while to build and they require a bunch of extra SDKs for mobile development).

And of course you want the code from this branch, feel free to ignore this section if you're familiar with git. It's very convenient if you have the github CLI installed, since you can just do this:

git clone https://github.com/scottplot/scottplot
cd scottplot
gh pr checkout 4648

If you don't have the github CLI I think you want this, but I have to admit I haven't done this in a while

git clone https://github.com/scottplot/scottplot
cd scottplot
git remote add bclehmann https://github.com/bclehmann/scottplot
git fetch bclehmann
git checkout --track bclehmann/fix/heatmap-rgba

You can also just clone my fork directly, and then you won't have to set up another remote:

git clone https://github.com/bclehmann/scottplot
cd scottplot
git fetch
git checkout --track origin/fix/heatmap-rgba

If you have the github desktop app you can also use the checkout with github desktop button:
image

If you did all of that right then running git log should show 197ee8ebe0351a5064c36e246e40592b06d657d5 as the first commit.

If you want to test in a NET notebook you can use ScottPlot.zip, just change yourfilepath/ScottPlot.dll to point to wherever you end up dropping the built dll.

Also note that there are both v4 and v5 ScottPlot solution files, you want v5.

@swharden
Copy link
Member

swharden commented Jan 4, 2025

I would like to validate it

@ssharks note that I'll be pushing a new version to NuGet approximately 24 hours from now, so you're welcome to verify it works as expected after going live 👍

@swharden swharden merged commit 832d176 into ScottPlot:main Jan 4, 2025
3 checks passed
@ssharks
Copy link

ssharks commented Jan 6, 2025

Thanks @bclehmann and @swharden. This morning I updated the NuGet package and was able to remove the workaround and have proper colors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Colors are swapped on platforms that default to ARGB instead of BRGA

3 participants