Ruleset.CreateIcon is inefficient in ruleset usages

[peppy]

It came to my attention that some rulesets are using sprites for their icons, creating a new TextureStore every call to CreateIcon(). This is super inefficient and may lead to memory exhaustion.

[peppy]

While looking into fixing this, we should also consider that calls to CreateIcon are dangerous, in that they may fail due to an API change (see #11912). There should probably be a path which can guarantee safety, at least at a constructor level. ie. rulesetStore.GetIconFor(ruleset), which provides a fallback icon if an exception is encountered).

[ayyEve]

like a default ruleset icon if no specific-ruleset icon is found?

the order [api -> local ruleset icon -> default ruleset icon] would make sense imo
this would give top priority to the api (most up to date image), if none is found try to use a locally defined ruleset icon (in case a ruleset-author wants their own default fallback), otherwise use the default icon (so theres always something available)

[peppy]

I don't think you understand what you're saying here. This doesn't need open discussion; it's an issue (causing potential crashes) purely at the osu.Game end and the comment above was more for myself than anyone else.

[ayyEve]

my mistake

[bdach]

Some rulesets have more proper logic that performs a once-off registration of their resource store at game-level. It looks like a viable alternative, at least when it comes to the performance side of things, but I'm not sure if I like it 100%. It's a bit of a scary change to make since namespacing will begin to be a problem.

I also thought about having a separate texture store for just the icons in RulesetStore, but that's also a bit ugly. It'll mean that CreateIcon() will have to be able to accept it via a parameter, which seems like a worse API than what we have. The alternate proposed path via rulesetStore.GetIconFor(ruleset) would hide that a bit (along with maybe making CreateIcon() protected internal), but I'm not sure how I'd feel about sticking a texture store in RulesetStore.

[peppy]

That kind of "proper logic" is literally game breaking, and would not be allowed in a ruleset which is approved for ranking etc. (polluting a global texture store with one provided by a ruleset). This could be used to manipulate gameplay outside of the ruleset itself, after all.

I'd say that we do want to cache or maintain resource stores locally in RulesetStore if they are required for this purpose.

[bdach]

Sure, I didn't intend to say it was completely correct, rather than it just being "more proper" from a performance standpoint. I understand the namespacing implications.