Refactor image search functions to use Get and TryGet terminology

[PunkPun]

fixes

[abcdefg30]

This seems weird. Why are we searching for non-existing images in the first place?

[PunkPun]

This seems weird. Why are we searching for non-existing images in the first place?

Because we want UI to support all state images without having a ton of redundant yaml

[dragunoff]

This seems weird. Why are we searching for non-existing images in the first place?

Because we want to avoid declaring chrome images for every state of an UI element (disabled, focused, pressed, hover) when we don't need them and fall back to the base image 🤷 The current plumbing seems not great at that and this PR is trying to work -around that but I feel like a bool flag is not the right call here.

[PunkPun]

The current plumbing seems not great at that and this PR is trying to work -around that but I feel like a bool flag is not the right call here.

I think bool flag is the only decent way to avoid code duplication. But perhaps we should move GetCachedStatefulImage and related functions to ChromeProvider.cs next to the GetImage funtion?

[abcdefg30]

Because we want to avoid declaring chrome images for every state of an UI element (disabled, focused, pressed, hover) when we don't need them and fall back to the base image 🤷 The current plumbing seems not great at that and this PR is trying to work -around that but I feel like a bool flag is not the right call here.

That sounds like the code should not be asking for every state then, doesn't it? 🤔

[PunkPun]

That sounds like the code should not be asking for every state then, doesn't it? 🤔

it's not, see GetImage and GetPanelImages functions. Both of them are altered in this pr

[RoosterDragon]

The flag makes sense to me, the goal of the lookups is to use a specific image if one is defined, but otherwise use the base image.

In this scenario if the specific image isn't defined we're expecting the base image to be used. So logging a message if the specific image is missing isn't helpful.

[PunkPun]

rebased

[pchote]

IMO exposing TryGetImage methods would present a cleaner API than the logNotFound bool.

[PunkPun]

We allow the program to function even if we didn't find the image, this is just for logging. If anything using TryGet would make it more confusing. However I'm not saying that the current way the system functions is correct / desired

[abcdefg30]

The problem with TryGet is that it moves logging the error to the caller, which we don't want in all cases. A compromise would be adding TryGet* methods but doing that while keeping the old Get* methods:

• Cases where we don't want to log an error can directly call the TryGet method
• Cases where we want a log to be written call the old Get method (so most of the callers see no change)
• Internally, the Get method utilizes TryGet and logs the error if TryGet returns false

Where the error the Get method handles is the one changed by the parameter in this PR. All other errors remain as-is.

[PunkPun]

separating into 2 methods requires duplication of code, or for us to keep the parameter which would defeat the purpose of having another function

[PunkPun]

But I could behind renaming GetImage to TryGetImage and GetPanelImages to TryGetPanelImages, because that's what they essentially do

[PunkPun]

Actually going through all the usecases of GetImage I see that we could just remove logging and crash instead. The code crashes regardless further on as null checks don't exist

[PunkPun]

I've refactored it to use the Get TryGet terminology, as well as made it crash on Get instead of just logging

[PunkPun]

rebased

[Mailaender]

Loading mod: modcontent
Exception of type System.ArgumentException: Sprite panel-rule/corner-tl was not found.
at OpenRA.Graphics.ChromeProvider.GetImage(String collectionName, String imageName) in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Game/Graphics/ChromeProvider.cs:line 150
at OpenRA.Graphics.ChromeProvider.TryGetPanelImages(String collectionName) in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Game/Graphics/ChromeProvider.cs:line 195
at OpenRA.Mods.Common.Widgets.BackgroundWidget.Draw() in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Mods.Common/Widgets/BackgroundWidget.cs:line 26
at OpenRA.Widgets.Widget.DrawOuter() in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Game/Widgets/Widget.cs:line 479
at OpenRA.Widgets.Widget.DrawOuter() in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Game/Widgets/Widget.cs:line 481
at OpenRA.Widgets.Widget.DrawOuter() in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Game/Widgets/Widget.cs:line 481
at OpenRA.Game.RenderTick() in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Game/Game.cs:line 733
at OpenRA.Game.Loop() in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Game/Game.cs:line 841
at OpenRA.Game.Run() in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Game/Game.cs:line 873
at OpenRA.Game.InitializeAndRun(String[] args) in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Game/Game.cs:line 308
at OpenRA.Launcher.Program.Main(String[] args) in /home/matthias/Entwicklung/OpenRA/OpenRA/OpenRA.Launcher/Program.cs:line 32

[PunkPun]

panel-rule seems to be as old as time itself (over 9 y. o.) and survived many ui related refactors, it either didn't work from the start or lost it's functionality somewhere years ago

[PunkPun]

My bad. After a long investigation into panel rendering code I've found out that panel-rule is written correctly and should in theory be functional. Though I've still yet to find the actual visual difference it makes by existing.

[abcdefg30]

Changelog