Progress reporting of async texture creation

[mlavik1]

Changes:

• Added progress reporting to data texture creation
• Added progress reporting to gradient texture creation
• Added progress bar to VolumeRenderedObject's inspector
• Removed ProgressHandler.Start and ProgressHandler.Finish
• Added async versions of setters for VolumeRenderedObject:
  • SetRenderModeAsync
  • SetTransferFunctionModeAsync
  • SetLightingEnabledAsync
  • SetTransferFunctionAsync
• Added currentStageProgress to IProgressView

#164

[mlavik1]

@SitronX Here's a PR for the issue we talked about.
Notice that I added a new currentStageProgress parameter to IProgressView. I guess you'll want to use that :)

I have also removed ProgressHandler.Start and ProgressHandler.Finish, so we don't have to "remember" to call those everywhere.. Also some places it's impossible to decide if we should call them or not (for example when creating TF texture, since that can happen as part of a longer process).
Now IProgressView.StartProgress is instead called from the constructor, and IProgressView.FinishProgress is called when progress reaches 100%.

[SitronX]

Hello @mlavik1
It looks good. But dont you also want to add reporting to default texture creation? I know it might seem pointless when you are working with small datasets, but in large datasets the classic texture creation takes a long time (on 730 slice dataset around 20 seconds, so the loader basically hangs up for 20 seconds showing 100% load state). I saw it is all prepared there, it just doesnt do reporting in these two expensive for cycles:

half variant

await Task.Run(() => {
    for (int iData = 0; iData < data.Length; iData++)
        pixelBytes[iData] = Mathf.FloatToHalf((float)(data[iData] - minValue) / maxRange);
});

float variant

await Task.Run(() => {
    for (int iData = 0; iData < data.Length; iData++)
        pixelBytes[iData] = (float)(data[iData] - minValue) / maxRange;
});

The gradient reporting is great, i like that progress bar :)

Also i have noticed weird scaling issue when not using simpleITK on one certain DICOM dataset. It is prolonged somehow, maybe you changed something recently?

With SimpleITK, the scale is alright like shown here

On sidenote. This is interesting code style, i didnt even know it is possible to throw away increment from for loop like this :D

private void CalculateValueBounds(IProgressHandler progressHandler)
{
   ...
        if (data != null)
        {
            int dimension = dimX * dimY * dimZ;
            int sliceDimension = dimX * dimY;
            for (int i = 0; i < dimension;)
            {
                for (int j = 0; j < sliceDimension; j++, i++)
                {
                    float val = data[i];
                    minDataValue = Mathf.Min(minDataValue, val);
                    maxDataValue = Mathf.Max(maxDataValue, val);
                }
                progressHandler.ReportProgress(i, dimension, "Calculating value bounds");
            }
        }

   ...
}

[mlavik1]

Hi @SitronX
Thanks for the feedback!

It looks good. But dont you also want to add reporting to default texture creation?

You're right, I should probably do it there as well. If I do it similar to how I did it in CalculateValueBounds then it shouldn't be too expensive either, so that's probably fine.

On sidenote. This is interesting code style, i didnt even know it is possible to throw away increment from for loop like this :D

Haha, thanks! 😁 I hesitated a bit about it, since it's not as intuitive, but do you think it's readable enough? It's probably a bit faster than the alternatives at least :)

Also i have noticed weird scaling issue when not using simpleITK on one certain DICOM dataset

Thanks for letting me know! I did change the openDICOM importer to do the same coordinate space conversion as the SimpleITK importer. I tested a few datasets and they looked the same as when imported with SimpleITK, so I'm not sure why this one looks like that.. Is it an open dataset that I can download somewhere maybe? :)

[mlavik1]

I've updated the progress reporting now!

And I've tried reproducing the issue with datasets having different scale in SimpleITK and OpenDICOM, but I've had no luck so far.. The ones I have get nearly the same scale. I did notice a small mistake in the importer (which is why they were only "nearly" the same. Can you try again now? If your dataset has very few slices then that might have been the cause.

[SitronX]

I did notice a small mistake in the importer (which is why they were only "nearly" the same. Can you try again now?

Yep, all good now. I tried it and it has correct scale now same as SimpleITK.

If your dataset has very few slices then that might have been the cause.

It was actually that huge 730 slice dataset that was having issues :D

Haha, thanks! 😁 I hesitated a bit about it, since it's not as intuitive, but do you think it's readable enough?

When i look back on it now, it is nice compact solution, but when i read it first, it took me a while to get what is really going on :D It also kinda suprised me, cause i never saw for cycle used like that :D If it was me i would probably swap that first for cycle with while loop, cause it basically acts like it and would have that iterator variable somewhere else. But that is just personal preference i guess. And plus side, your solution teaches people that for cycle can also be used like that :D

It's probably a bit faster than the alternatives at least :)

Ye definetely, this is better, cause you are not checking the value everytime in that loop like me :) Although as i previously said somewhere, when i tested my variant, it added only negligable additional time (12s with reporting/10s without).

I've updated the progress reporting now!

Yep, now it looks great, thanks. PR has my blessing now haha :D

[mlavik1]

Yep, all good now. I tried it and it has correct scale now same as SimpleITK.

Great to hear! Thanks for testing :D

If it was me i would probably swap that first for cycle with while loop

Yes that could work too! I'll look through all this again before I set up the next release, so I'll consider changing it then.

Ye definetely, this is better, cause you are not checking the value everytime in that loop like me :) Although as i previously said somewhere, when i tested my variant, it added only negligable additional time (12s with reporting/10s without).

Yes, probably your progress reporting callback function is pretty fast then :) If it were slower it could be more of an issue. But I guess one callback per slice should be enough 😁

Yep, now it looks great, thanks. PR has my blessing now haha :D

Thanks once again for your help! I'll merge this then!