From 969783e8b252f30b005497e677763148a1eec1f7 Mon Sep 17 00:00:00 2001 From: eero pomell Date: Tue, 28 May 2024 02:25:43 +0300 Subject: [PATCH 1/9] Fix issue where images that are too large fail to load without any informative message (#688) --- .../ReferencePanelTabImage.prefab | 162 ++++++++++++++++++ .../Scripts/GUI/LoadReferenceImageButton.cs | 35 ++-- .../Strings/Strings Shared Data.asset | 4 + .../Localization/Strings/Strings_en.asset | 6 +- 4 files changed, 193 insertions(+), 14 deletions(-) diff --git a/Assets/Prefabs/Panels/ReferencePanel/ReferencePanelTabImage.prefab b/Assets/Prefabs/Panels/ReferencePanel/ReferencePanelTabImage.prefab index 07682e8512..c827c25db0 100644 --- a/Assets/Prefabs/Panels/ReferencePanel/ReferencePanelTabImage.prefab +++ b/Assets/Prefabs/Panels/ReferencePanel/ReferencePanelTabImage.prefab @@ -121,6 +121,24 @@ MonoBehaviour: m_HoverScale: 1.2 m_HoverBoxColliderGrow: 0.2 m_AddOverlay: 0 + m_LoadHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238555522805800960 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] + m_ErrorHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238556149774557184 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] references: version: 2 RefIds: [] @@ -207,6 +225,24 @@ MonoBehaviour: m_HoverScale: 1.2 m_HoverBoxColliderGrow: 0.2 m_AddOverlay: 0 + m_LoadHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238555522805800960 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] + m_ErrorHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238556149774557184 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] references: version: 2 RefIds: [] @@ -262,6 +298,24 @@ MonoBehaviour: m_HoverScale: 1.2 m_HoverBoxColliderGrow: 0.2 m_AddOverlay: 0 + m_LoadHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238555522805800960 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] + m_ErrorHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238556149774557184 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] references: version: 2 RefIds: [] @@ -317,6 +371,24 @@ MonoBehaviour: m_HoverScale: 1.2 m_HoverBoxColliderGrow: 0.2 m_AddOverlay: 0 + m_LoadHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238555522805800960 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] + m_ErrorHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238556149774557184 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] references: version: 2 RefIds: [] @@ -372,6 +444,24 @@ MonoBehaviour: m_HoverScale: 1.2 m_HoverBoxColliderGrow: 0.2 m_AddOverlay: 0 + m_LoadHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238555522805800960 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] + m_ErrorHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238556149774557184 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] references: version: 2 RefIds: [] @@ -427,6 +517,24 @@ MonoBehaviour: m_HoverScale: 1.2 m_HoverBoxColliderGrow: 0.2 m_AddOverlay: 0 + m_LoadHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238555522805800960 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] + m_ErrorHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238556149774557184 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] references: version: 2 RefIds: [] @@ -482,6 +590,24 @@ MonoBehaviour: m_HoverScale: 1.2 m_HoverBoxColliderGrow: 0.2 m_AddOverlay: 0 + m_LoadHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238555522805800960 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] + m_ErrorHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238556149774557184 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] references: version: 2 RefIds: [] @@ -537,6 +663,24 @@ MonoBehaviour: m_HoverScale: 1.2 m_HoverBoxColliderGrow: 0.2 m_AddOverlay: 0 + m_LoadHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238555522805800960 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] + m_ErrorHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238556149774557184 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] references: version: 2 RefIds: [] @@ -592,6 +736,24 @@ MonoBehaviour: m_HoverScale: 1.2 m_HoverBoxColliderGrow: 0.2 m_AddOverlay: 0 + m_LoadHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238555522805800960 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] + m_ErrorHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238556149774557184 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] references: version: 2 RefIds: [] diff --git a/Assets/Scripts/GUI/LoadReferenceImageButton.cs b/Assets/Scripts/GUI/LoadReferenceImageButton.cs index cbc222bfb3..71e8551b56 100644 --- a/Assets/Scripts/GUI/LoadReferenceImageButton.cs +++ b/Assets/Scripts/GUI/LoadReferenceImageButton.cs @@ -12,6 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. +using UnityEngine.Localization; +using UnityEngine; + namespace TiltBrush { @@ -19,17 +22,30 @@ public class LoadReferenceImageButton : BaseButton { public ReferenceImage ReferenceImage { get; set; } + [SerializeField] private LocalizedString m_ErrorHelpText; + + + public void RefreshDescription() { if (ReferenceImage != null) { - SetDescriptionText(ReferenceImage.FileName); + + if (!ReferenceImage.Valid) + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); + } + else + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); + } + } } override protected void OnButtonPressed() { - if (ReferenceImage == null) + if (ReferenceImage == null || !ReferenceImage.Valid) { return; } @@ -56,18 +72,11 @@ override protected void OnButtonPressed() override public void ResetState() { base.ResetState(); + } - // Make ourselves unavailable if our image has an error. - bool available = false; - if (ReferenceImage != null) - { - available = ReferenceImage.NotLoaded || ReferenceImage.Valid; - } - - if (available != IsAvailable()) - { - SetButtonAvailable(available); - } + public string ImageErrorExtraDescription() + { + return m_ErrorHelpText.GetLocalizedStringAsync().Result; } } } // namespace TiltBrush diff --git a/Assets/Settings/Localization/Strings/Strings Shared Data.asset b/Assets/Settings/Localization/Strings/Strings Shared Data.asset index 82bf2e5130..c0578650b5 100644 --- a/Assets/Settings/Localization/Strings/Strings Shared Data.asset +++ b/Assets/Settings/Localization/Strings/Strings Shared Data.asset @@ -3315,6 +3315,10 @@ MonoBehaviour: m_Key: DirectoryChooserPopupButton m_Metadata: m_Items: [] + - m_Id: 238556149774557184 + m_Key: PANEL_REFERENCE_ICONIMAGE_LOADERRORTEXT + m_Metadata: + m_Items: [] m_Metadata: m_Items: [] m_KeyGenerator: diff --git a/Assets/Settings/Localization/Strings/Strings_en.asset b/Assets/Settings/Localization/Strings/Strings_en.asset index f85b1273b4..57c5e98741 100644 --- a/Assets/Settings/Localization/Strings/Strings_en.asset +++ b/Assets/Settings/Localization/Strings/Strings_en.asset @@ -11,7 +11,7 @@ MonoBehaviour: m_EditorHideFlags: 0 m_Script: {fileID: 11500000, guid: e9620f8c34305754d8cc9a7e49e852d9, type: 3} m_Name: Strings_en - m_EditorClassIdentifier: + m_EditorClassIdentifier: m_LocaleId: m_Code: en m_SharedData: {fileID: 11400000, guid: c84355079ab3f3e4f8f3812258805f86, type: 2} @@ -3512,6 +3512,10 @@ MonoBehaviour: m_Localized: Pick a Subfolder m_Metadata: m_Items: [] + - m_Id: 238556149774557184 + m_Localized: Image too large to load + m_Metadata: + m_Items: [] references: version: 2 RefIds: [] From 85bedb94645b17092c1e94fc9012e6028d8119f0 Mon Sep 17 00:00:00 2001 From: eero pomell Date: Wed, 29 May 2024 02:11:42 +0300 Subject: [PATCH 2/9] Move image description functionality from RefreshDescription() to ResetState() --- .../Scripts/GUI/LoadReferenceImageButton.cs | 40 +++++++++++++------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/Assets/Scripts/GUI/LoadReferenceImageButton.cs b/Assets/Scripts/GUI/LoadReferenceImageButton.cs index 71e8551b56..67d6b52c65 100644 --- a/Assets/Scripts/GUI/LoadReferenceImageButton.cs +++ b/Assets/Scripts/GUI/LoadReferenceImageButton.cs @@ -25,22 +25,24 @@ public class LoadReferenceImageButton : BaseButton [SerializeField] private LocalizedString m_ErrorHelpText; - + // this is commented out and moved into ResetState() because + // for a new image (ie., one that isn't in the cache yet), !ReferenceImage.Valid, even if the size is valid (ie less than the max size) + // TODO: figure out if it's bad to move this into ResetState() public void RefreshDescription() { - if (ReferenceImage != null) - { + /* if (ReferenceImage != null) + { - if (!ReferenceImage.Valid) - { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); - } - else - { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); - } + if (!ReferenceImage.Valid) + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); + } + else + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); + } - } + }*/ } override protected void OnButtonPressed() @@ -72,6 +74,20 @@ override protected void OnButtonPressed() override public void ResetState() { base.ResetState(); + + if (ReferenceImage == null) + { + return; + } + + if (!ReferenceImage.Valid) + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); + } + else + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); + } } public string ImageErrorExtraDescription() From 1745ab46d2e0e49a811fb209d267175c1dda6054 Mon Sep 17 00:00:00 2001 From: eero pomell Date: Wed, 29 May 2024 02:59:02 +0300 Subject: [PATCH 3/9] Add image load size informative error message to background images panel --- .../ReferencePanelIconBackgroundImage.prefab | 9 ++++ .../Scripts/GUI/LoadBackgroundImageButton.cs | 47 ++++++++++++++----- .../Strings/Strings Shared Data.asset | 4 ++ .../Localization/Strings/Strings_en.asset | 4 ++ 4 files changed, 52 insertions(+), 12 deletions(-) diff --git a/Assets/Prefabs/Panels/ReferencePanel/ReferencePanelIconBackgroundImage.prefab b/Assets/Prefabs/Panels/ReferencePanel/ReferencePanelIconBackgroundImage.prefab index 03cd6dfe23..8ebc10ae21 100644 --- a/Assets/Prefabs/Panels/ReferencePanel/ReferencePanelIconBackgroundImage.prefab +++ b/Assets/Prefabs/Panels/ReferencePanel/ReferencePanelIconBackgroundImage.prefab @@ -144,6 +144,15 @@ MonoBehaviour: m_HoverScale: 1.2 m_HoverBoxColliderGrow: 0.2 m_AddOverlay: 0 + m_ErrorHelpText: + m_TableReference: + m_TableCollectionName: GUID:c84355079ab3f3e4f8f3812258805f86 + m_TableEntryReference: + m_KeyId: 238935604850335744 + m_Key: + m_FallbackState: 0 + m_WaitForCompletion: 0 + m_LocalVariables: [] references: version: 2 RefIds: [] diff --git a/Assets/Scripts/GUI/LoadBackgroundImageButton.cs b/Assets/Scripts/GUI/LoadBackgroundImageButton.cs index e4af884b2e..2b246f4caf 100644 --- a/Assets/Scripts/GUI/LoadBackgroundImageButton.cs +++ b/Assets/Scripts/GUI/LoadBackgroundImageButton.cs @@ -13,6 +13,8 @@ // limitations under the License. using UnityEngine; +using UnityEngine.Localization; + namespace TiltBrush { @@ -20,17 +22,30 @@ public class LoadBackgroundImageButton : BaseButton { public ReferenceImage ReferenceImage { get; set; } + [SerializeField] private LocalizedString m_ErrorHelpText; + + // this is commented out and moved into ResetState() because + // for a new image (ie., one that isn't in the cache yet), !ReferenceImage.Valid, even if the size is valid (ie less than the max size) + // TODO: figure out if it's bad to move this into ResetState() public void RefreshDescription() { - if (ReferenceImage != null) - { - SetDescriptionText(ReferenceImage.FileName); - } - } + /* if (ReferenceImage != null) + { + + if (!ReferenceImage.Valid) + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); + } + else + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); + } + }*/ + } override protected void OnButtonPressed() { - if (ReferenceImage == null) + if (ReferenceImage == null || !ReferenceImage.Valid) { return; } @@ -41,16 +56,18 @@ override public void ResetState() { base.ResetState(); - // Make ourselves unavailable if our image has an error. - bool available = false; - if (ReferenceImage != null) + if (ReferenceImage == null) { - available = ReferenceImage.NotLoaded || ReferenceImage.Valid; + return; } - if (available != IsAvailable()) + if (!ReferenceImage.Valid) + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); + } + else { - SetButtonAvailable(available); + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); } } @@ -62,5 +79,11 @@ public void Set360ButtonTexture(Texture2D rTexture, float aspect = -1) m_ButtonRenderer.material.SetFloat("_Stereoscopic", isStereo); } + + public string ImageErrorExtraDescription() + { + return m_ErrorHelpText.GetLocalizedStringAsync().Result; + } + } } // namespace TiltBrush diff --git a/Assets/Settings/Localization/Strings/Strings Shared Data.asset b/Assets/Settings/Localization/Strings/Strings Shared Data.asset index c0578650b5..c160912a3a 100644 --- a/Assets/Settings/Localization/Strings/Strings Shared Data.asset +++ b/Assets/Settings/Localization/Strings/Strings Shared Data.asset @@ -3319,6 +3319,10 @@ MonoBehaviour: m_Key: PANEL_REFERENCE_ICONIMAGE_LOADERRORTEXT m_Metadata: m_Items: [] + - m_Id: 238935604850335744 + m_Key: PANEL_REFERENCE_ICONBACKGROUNDIMAGE_LOADERRORTEXT + m_Metadata: + m_Items: [] m_Metadata: m_Items: [] m_KeyGenerator: diff --git a/Assets/Settings/Localization/Strings/Strings_en.asset b/Assets/Settings/Localization/Strings/Strings_en.asset index 57c5e98741..a6880379f4 100644 --- a/Assets/Settings/Localization/Strings/Strings_en.asset +++ b/Assets/Settings/Localization/Strings/Strings_en.asset @@ -3516,6 +3516,10 @@ MonoBehaviour: m_Localized: Image too large to load m_Metadata: m_Items: [] + - m_Id: 238935604850335744 + m_Localized: Image too large to load + m_Metadata: + m_Items: [] references: version: 2 RefIds: [] From a66aa9683110db9e2c12a2108801d4616e4193f6 Mon Sep 17 00:00:00 2001 From: eero pomell Date: Wed, 29 May 2024 21:09:26 +0300 Subject: [PATCH 4/9] Change description setting logic in LoadReferenceImageButton.cs (#688) This is an improvement from the 2 previous attempts (commits) at fixing issue #688 because it modifies the original logic way less. We just make ResetState() call RefreshDescription() every time. However, this version still has atleast one problem: - an image can have an error for other reasons too, not just for being too large. We're currently checking if the image has *any* error. (I think we need to create a way to check for image size error?) --- .../Scripts/GUI/LoadReferenceImageButton.cs | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/Assets/Scripts/GUI/LoadReferenceImageButton.cs b/Assets/Scripts/GUI/LoadReferenceImageButton.cs index 67d6b52c65..2892e31cfc 100644 --- a/Assets/Scripts/GUI/LoadReferenceImageButton.cs +++ b/Assets/Scripts/GUI/LoadReferenceImageButton.cs @@ -24,30 +24,28 @@ public class LoadReferenceImageButton : BaseButton [SerializeField] private LocalizedString m_ErrorHelpText; - - // this is commented out and moved into ResetState() because - // for a new image (ie., one that isn't in the cache yet), !ReferenceImage.Valid, even if the size is valid (ie less than the max size) - // TODO: figure out if it's bad to move this into ResetState() public void RefreshDescription() { - /* if (ReferenceImage != null) - { + if (ReferenceImage != null) + { - if (!ReferenceImage.Valid) - { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); - } - else - { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); - } + // 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) + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); + } + else + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); + } - }*/ + } } override protected void OnButtonPressed() { - if (ReferenceImage == null || !ReferenceImage.Valid) + if (ReferenceImage == null) { return; } @@ -75,19 +73,19 @@ override public void ResetState() { base.ResetState(); - if (ReferenceImage == null) + // Make ourselves unavailable if our image has an error. + bool available = false; + if (ReferenceImage != null) { - return; + available = ReferenceImage.NotLoaded || ReferenceImage.Valid; } - if (!ReferenceImage.Valid) + if (available != IsAvailable()) { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); - } - else - { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); + SetButtonAvailable(available); } + + RefreshDescription(); } public string ImageErrorExtraDescription() From 08e43189dee99cd22c703400cdacac0906626bb9 Mon Sep 17 00:00:00 2001 From: eero pomell Date: Fri, 7 Jun 2024 15:12:32 +0300 Subject: [PATCH 5/9] Add generic error message string for images 'Image failed to load' --- .../Settings/Localization/Strings/Strings Shared Data.asset | 4 ++++ Assets/Settings/Localization/Strings/Strings_en.asset | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/Assets/Settings/Localization/Strings/Strings Shared Data.asset b/Assets/Settings/Localization/Strings/Strings Shared Data.asset index c160912a3a..c60ef0817e 100644 --- a/Assets/Settings/Localization/Strings/Strings Shared Data.asset +++ b/Assets/Settings/Localization/Strings/Strings Shared Data.asset @@ -3323,6 +3323,10 @@ MonoBehaviour: m_Key: PANEL_REFERENCE_ICONBACKGROUNDIMAGE_LOADERRORTEXT m_Metadata: m_Items: [] + - m_Id: 242372529028341760 + m_Key: PANEL_REFERENCE_ICONIMAGE_GENERICERRORTEXT + m_Metadata: + m_Items: [] m_Metadata: m_Items: [] m_KeyGenerator: diff --git a/Assets/Settings/Localization/Strings/Strings_en.asset b/Assets/Settings/Localization/Strings/Strings_en.asset index a6880379f4..dc601834e7 100644 --- a/Assets/Settings/Localization/Strings/Strings_en.asset +++ b/Assets/Settings/Localization/Strings/Strings_en.asset @@ -3520,6 +3520,10 @@ MonoBehaviour: m_Localized: Image too large to load m_Metadata: m_Items: [] + - m_Id: 242372529028341760 + m_Localized: Image failed to load + m_Metadata: + m_Items: [] references: version: 2 RefIds: [] From b053d2e7f68e08634c722891588c9cb938f09ca5 Mon Sep 17 00:00:00 2001 From: eero pomell Date: Fri, 7 Jun 2024 15:27:45 +0300 Subject: [PATCH 6/9] add ImageErrorExtraDescription() to ReferenceImage for getting image load error-specific message - Add a new entry to the ImageState enum for the error where an image is too large to load (ErrorImageTooLarge) - Only 2 error messages right now: a generic one "Image failed to load" and for images that are too large "Image too large to load" - Move ImageErrorExtraDescription() from LoadReferenceImageButton.cs into ReferenceImage.cs, because it needs access to the error states. --- .../Scripts/GUI/LoadReferenceImageButton.cs | 16 +++------ Assets/Scripts/ReferenceImage.cs | 34 +++++++++++++++++-- 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/Assets/Scripts/GUI/LoadReferenceImageButton.cs b/Assets/Scripts/GUI/LoadReferenceImageButton.cs index 2892e31cfc..6c0aa2da30 100644 --- a/Assets/Scripts/GUI/LoadReferenceImageButton.cs +++ b/Assets/Scripts/GUI/LoadReferenceImageButton.cs @@ -22,18 +22,17 @@ public class LoadReferenceImageButton : BaseButton { public ReferenceImage ReferenceImage { get; set; } - [SerializeField] private LocalizedString m_ErrorHelpText; - 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) + // null if image doesn't have error + string errorMessage = ReferenceImage.ImageErrorExtraDescription(); + + if (errorMessage != null) { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), errorMessage); } else { @@ -87,10 +86,5 @@ override public void ResetState() RefreshDescription(); } - - public string ImageErrorExtraDescription() - { - return m_ErrorHelpText.GetLocalizedStringAsync().Result; - } } } // namespace TiltBrush diff --git a/Assets/Scripts/ReferenceImage.cs b/Assets/Scripts/ReferenceImage.cs index 420b4fa2d8..572ca161af 100644 --- a/Assets/Scripts/ReferenceImage.cs +++ b/Assets/Scripts/ReferenceImage.cs @@ -19,6 +19,8 @@ using Unity.VectorGraphics; using Superla.RadianceHDR; using UnityEngine; +using UnityEngine.Localization; +using UnityEngine.Localization.Settings; namespace TiltBrush { @@ -37,6 +39,10 @@ private enum ImageState // Same meaning as Future.State.Error // Invariant: m_coroutine == null Error, + // This is the only specific error message right now. ("Image too large to load") + // For other errors (e.g unknown format), we set Error state and display a generic error message "Image failed to load" + // ImageUtils.cs throws more specific errors, and we could implement them here as well in the future. + ErrorImageTooLarge } // See ImageState for invariants @@ -49,6 +55,10 @@ private enum ImageState private string m_Path; private SVGParser.SceneInfo _SvgSceneInfo; + private LocalizedString m_ErrorImageTooLargeHelpText = new LocalizedString("Strings", "PANEL_REFERENCE_ICONIMAGE_LOADERRORTEXT"); + private LocalizedString m_ErrorGenericHelpText = new LocalizedString("Strings", "PANEL_REFERENCE_ICONIMAGE_GENERICERRORTEXT"); + + // public bool IsComposite => _SvgSceneInfo.Scene.Root.getsh public string FileName { get { return Path.GetFileName(m_Path); } } @@ -71,6 +81,23 @@ public float ImageAspect } } + // returns null if no error in image + public string ImageErrorExtraDescription() + { + if (m_State != ImageState.Error && m_State != ImageState.ErrorImageTooLarge) + { + return null; + } + else if (m_State == ImageState.Error) + { + return m_ErrorGenericHelpText.GetLocalizedStringAsync().Result; + } + else + { + return m_ErrorImageTooLargeHelpText.GetLocalizedStringAsync().Result; + } + } + public bool NotLoaded { get { return m_State == ImageState.Uninitialized || m_State == ImageState.NotReady; } @@ -93,7 +120,8 @@ public Texture2D Icon switch (m_State) { case ImageState.Ready: return m_Icon; - case ImageState.Error: return ReferenceImageCatalog.m_Instance.ErrorImage; + case ImageState.Error: + case ImageState.ErrorImageTooLarge: return ReferenceImageCatalog.m_Instance.ErrorImage; default: case ImageState.Uninitialized: case ImageState.NotReady: return null; @@ -290,7 +318,7 @@ public bool RequestLoad(bool allowMainThread = false) // If this file is too large for the platform, don't load it. if (!ValidateFileSize()) { - m_State = ImageState.Error; + m_State = ImageState.ErrorImageTooLarge; ControllerConsoleScript.m_Instance.AddNewLine( FileName + " is too large and could not be loaded.", true); @@ -550,6 +578,8 @@ IEnumerable RequestLoadCoroutine() else { // Problem reading the file? + // images with state ImageState.Error display a generic error message 'Image failed to load' when hovering over them in the reference panel + // TODO: use more specific error messages (e.g "Too large dimensions", "Unknown format") that are set in ImageUtils.cs? (that is called by ThreadedImageReader.cs) m_State = ImageState.Error; reader = null; yield break; From 1f98ca398dbccb7ab729ae0bd7857e862610678c Mon Sep 17 00:00:00 2001 From: eero pomell Date: Mon, 10 Jun 2024 20:14:17 +0300 Subject: [PATCH 7/9] Update background image loading to also show the new informative error messages --- .../Scripts/GUI/LoadBackgroundImageButton.cs | 52 ++++++++----------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/Assets/Scripts/GUI/LoadBackgroundImageButton.cs b/Assets/Scripts/GUI/LoadBackgroundImageButton.cs index 2b246f4caf..c29a01d0f1 100644 --- a/Assets/Scripts/GUI/LoadBackgroundImageButton.cs +++ b/Assets/Scripts/GUI/LoadBackgroundImageButton.cs @@ -22,26 +22,24 @@ public class LoadBackgroundImageButton : BaseButton { public ReferenceImage ReferenceImage { get; set; } - [SerializeField] private LocalizedString m_ErrorHelpText; - - // this is commented out and moved into ResetState() because - // for a new image (ie., one that isn't in the cache yet), !ReferenceImage.Valid, even if the size is valid (ie less than the max size) - // TODO: figure out if it's bad to move this into ResetState() public void RefreshDescription() { - /* if (ReferenceImage != null) - { + if (ReferenceImage != null) + { + + // null if image doesn't have error + string errorMessage = ReferenceImage.ImageErrorExtraDescription(); - if (!ReferenceImage.Valid) - { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); - } - else - { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); - } + if (errorMessage != null) + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), errorMessage); + } + else + { + SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); + } - }*/ + } } override protected void OnButtonPressed() { @@ -56,19 +54,19 @@ override public void ResetState() { base.ResetState(); - if (ReferenceImage == null) + // Make ourselves unavailable if our image has an error. + bool available = false; + if (ReferenceImage != null) { - return; + available = ReferenceImage.NotLoaded || ReferenceImage.Valid; } - if (!ReferenceImage.Valid) - { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName), ImageErrorExtraDescription()); - } - else + if (available != IsAvailable()) { - SetDescriptionText(App.ShortenForDescriptionText(ReferenceImage.FileName)); + SetButtonAvailable(available); } + + RefreshDescription(); } public void Set360ButtonTexture(Texture2D rTexture, float aspect = -1) @@ -79,11 +77,5 @@ public void Set360ButtonTexture(Texture2D rTexture, float aspect = -1) m_ButtonRenderer.material.SetFloat("_Stereoscopic", isStereo); } - - public string ImageErrorExtraDescription() - { - return m_ErrorHelpText.GetLocalizedStringAsync().Result; - } - } } // namespace TiltBrush From dedda347b65ad1999d6a6d611cf800d12063e565 Mon Sep 17 00:00:00 2001 From: eero pomell Date: Mon, 10 Jun 2024 20:21:06 +0300 Subject: [PATCH 8/9] Add enum code to new enum entry ErrorImageTooLarge because it is required when adding new items to enums --- Assets/Scripts/ReferenceImage.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Assets/Scripts/ReferenceImage.cs b/Assets/Scripts/ReferenceImage.cs index 572ca161af..75afa786fe 100644 --- a/Assets/Scripts/ReferenceImage.cs +++ b/Assets/Scripts/ReferenceImage.cs @@ -42,7 +42,7 @@ private enum ImageState // This is the only specific error message right now. ("Image too large to load") // For other errors (e.g unknown format), we set Error state and display a generic error message "Image failed to load" // ImageUtils.cs throws more specific errors, and we could implement them here as well in the future. - ErrorImageTooLarge + ErrorImageTooLarge = 31000 } // See ImageState for invariants From 53dd256ded3076271518b88fa09aae9e747ef21f Mon Sep 17 00:00:00 2001 From: eero pomell Date: Mon, 10 Jun 2024 20:36:21 +0300 Subject: [PATCH 9/9] Remove not needed logic in LoadBackgroundImageButton.OnButtonPressed() Not needed anymore as ResetState() sets the button unavailable if the image is invalid --- Assets/Scripts/GUI/LoadBackgroundImageButton.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Assets/Scripts/GUI/LoadBackgroundImageButton.cs b/Assets/Scripts/GUI/LoadBackgroundImageButton.cs index c29a01d0f1..d5c0e46b85 100644 --- a/Assets/Scripts/GUI/LoadBackgroundImageButton.cs +++ b/Assets/Scripts/GUI/LoadBackgroundImageButton.cs @@ -43,7 +43,7 @@ public void RefreshDescription() } override protected void OnButtonPressed() { - if (ReferenceImage == null || !ReferenceImage.Valid) + if (ReferenceImage == null) { return; }