From 85bedb94645b17092c1e94fc9012e6028d8119f0 Mon Sep 17 00:00:00 2001 From: eero pomell Date: Wed, 29 May 2024 02:11:42 +0300 Subject: [PATCH 1/3] 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 71e8551b5..67d6b52c6 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 2/3] 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 03cd6dfe2..8ebc10ae2 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 e4af884b2..2b246f4ca 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 c0578650b..c160912a3 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 57c5e9874..a6880379f 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 3/3] 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 67d6b52c6..2892e31cf 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()