Fix issue where images that are too large fail to load without any in…

[eeropomell]

The UI can still be changed, but currently it is using the TwoLine tooltip button description as suggested by Mike. So for images that are too large, it displays:
"00001.png
Image too large to load"

And for images that are valid it displays the same as previously:
"00001.png"

2024-05-28.02-01-01.1.mp4

I haven't tested everything fully yet. For example, I'm not sure if the removal of basically all code in ResetState() in LoadReferenceImageButton breaks something. In the previous version, the code in ResetState() was checking if the image had an error, and if yes, set it unavailable (so OnButtonPressed() wouldn't get called for the image when pressed). In the new version, the logic is moved into OnButtonPressed.

[eeropomell]

Also, I still need to check the Quest size limits.

[mikeage]

thanks for the PR! Just so you know, all first time contributors require manual approval before Github actions run (despite the fact that they ran on your fork), but it's been approved and shouldn't pose any problem going forward.

[eeropomell]

In the issue, Andy mentioned that:
"Check to see if our limits for the Quest still make sense."
and that
"Especially an issue with background images as people want to push the resolution for those nice and high. 6k seems to work. Not sure beyond that."

How do we go about figuring out the correct limits for the Quest? I just tried on my quest 3, and I can upload 8K images without crashing. Is it going to be as simple as figuring out a size that causes a crash or do we do some profiling?

[mikeage]

Huh, apparently we still need to manually approve CI runs.

[andybak]

ImageButton.RefreshDescription is called in Refresh() in RefreshImageButtonTab so if that no longer updates the description, does it need to do something else (like call RefreshState()?)

I must admit I'd be tempted to find a way to do this without moving the logic around too much but it's up to you which way you want to go. I'd rather have some code duplication than have to spend ages tracing the flow to make sure I hadn't broken anything else!

[andybak]

In the issue, Andy mentioned that:
"Check to see if our limits for the Quest still make sense."
and that
"Especially an issue with background images as people want to push the resolution for those nice and high. 6k seems to work. Not sure beyond that."

I'd suggest we leave this for a separate PR. Certainly avoid discussing here at the moment until we've got the more directly relevant stuff ironed out.

[eeropomell]

I agree with:

I must admit I'd be tempted to find a way to do this without moving the logic around too much but it's up to you which way you want to go. I'd rather have some code duplication than have to spend ages tracing the flow to make sure I hadn't broken anything else!

I updated the logic to be more similar to the original version. In this one, we call RefreshDescription() from ResetState() every time. It's an improvement from the previous versions, but there's at least one problem currently:

We check for any error in the image, when we should check for only the error where the image is too large. (I think we need to add a way to do this)

In this new version, we're checking for an error with ReferenceImage.Icon == ReferenceImageCatalog.m_Instance.ErrorImage:

        public void RefreshDescription()
        {
            if (ReferenceImage != null)
            {

                // Problem: image can have error for other reasons too, not just for being too large
                // displays "Image too large to load" under the file name.
                if (ReferenceImage.Icon == ReferenceImageCatalog.m_Instance.ErrorImage)
                {

In the previous version it was !ReferenceImage.Valid, but images that are less than the max size, will be !ReferenceImage.Valid before being loaded.

Icon is defined like this in ReferenceImage:

  public Texture2D Icon
        {
            get
            {
                switch (m_State)
                {
                    case ImageState.Ready: return m_Icon;
                    case ImageState.Error: return ReferenceImageCatalog.m_Instance.ErrorImage;
                    default:
                    case ImageState.Uninitialized:
                    case ImageState.NotReady: return null;
                }
            }
        }

The current problem is that an image can have an error for other reasons too, not just for being too large. (the Error state is set in 2 places in ReferenceImage, once in RequestLoad() if too large, and once in RequestLoadCoroutine() if there's an error reading the file)

Should we add a new public method in ReferenceImage to check if the image has an error and what kind of an error? For example, Valid is defined as:
public bool Valid { get { return m_State == ImageState.Ready; } }

[eeropomell]

also, if you're wondering why I just force pushed an edit to the latest commit message, it's because I realized that the 2nd problem (regarding the possibility that an image too large being in NotReady or Unitialized states before the Error state being a problem) I mention is probably not a problem, after looking at the code RequestLoad() code more.

In ReferencePanelImageTab.cs:

       public override void RefreshTab(bool selected)
        {
            base.RefreshTab(selected);
            if (selected)
            {
                if (m_AutoLoadImages)
                {
                    ReferenceImageCatalog.m_Instance.RequestLoadImages(m_IndexOffset,
                        m_IndexOffset + m_Icons.Length);
                }
                m_AllIconTexturesAssigned = false;
            }
        }

So if m_AutoLoadImages is true (which it is), then the images are loaded (by RequestLoad()) every time the tab changes.

i.e., when the player opens the image tab, or switches the page, then the images will load, and images that are too large will be set to the Error state. I don't think there's a possibility of the player being able to press on a large image icon before it is set to the Error state.

[andybak]

How about setting the error type in RequestLoad() or whenever you know the specific reason for the error - and then use this to pick the correct error message later in ImageErrorExtraDescription() ? (Or set the text directly in RequestLoad)

[eeropomell]

Sorry for the accidental closing of the PR.

How about setting the error type in RequestLoad() or whenever you know the specific reason for the error - and then use this to pick the correct error message later in ImageErrorExtraDescription() ? (Or set the text directly in RequestLoad)

I re-opened it and implemented this now. RequestLoad() sets an error state for when the image is too large, and for other errors it sets just a generic error (the one that has previously been used for all errors). I added a new entry to the ImageState enum called ErrorImageTooLarge (I think I need to add an enum code to it and to the spreadsheet, before this can be merged, right?)

In the current version, for images that have too large file size, the user will see "Image too large to load", and for other errors such as unknown format, the user will see "Image failed to load".

Haven't yet copied these changes to the background image reference panel.

ImageUtils.cs throws more specific errors for images, like "Unknown format" and "Too large dimensions" and "Can't get dimensions from unknown image format!"

From the user's perspective, do we want to show all these specific error messages or just an "Image failed to load"?

[eeropomell]

I've just noticed that ReferenceImage doesn't set the generic error in all cases. RequestLoad() calls either RequestLoadCoroutineMainThread or RequestLoadCoroutine, and only RequestLoadCoroutine sets the error state in case there's an error. RequestLoadCoroutineMainThread will load any dimension image without checking if it goes beyond the limits.

However I'd say we focus on implementing and merging the file size error first. I can look into this after that.

[eeropomell]

I just did 2 things:

• updated background image loading so it also shows informative error messages
• added an enum code to the new item in ImageState, and also reserved a block in the spreadsheet.

Ready for merging now, I think.

[andybak]

All looks great. Thanks!