ResourcePack: Avoid crashes on invalid packs during Init().

[AdmiralCurtiss]

Someone on Discord had an issue where Dolphin would crash immediately when opening it, and this turned out to be the cause. If you have a .zip file in the ResourcePacks folder that doesn't contain a manifest, Init() dereferences a null pointer trying to sort them.

While I was here I also adjusted the code so it doesn't re-parse the zip files on every single comparison operation.

[AdmiralCurtiss]

I know basically no one uses resource packs but this is still a very confusing crash for those who run into it. Can we get this reviewed/merged?

[AdmiralCurtiss]

If no one cares then I'll just merge this one of these days, okay?

[Dentomologist]

I believe the way you're using emplace_back here is equivalent to push_back. With both examples being proceded by this for clarity:

const std::string id_or_name = pack.IsValid() ? pack.GetManifest()->GetID() : pack_list[i];

You would want to do

pack_list_order.emplace_back(i, id_or_name);

(which invokes the OrderHelper(size_t, std::string) constructor) instead of

pack_list_order.emplace_back(OrderHelper{i, id_or_name});

(which first invokes the OrderHelper(size_t, std::string) constructor to create a temporary object, then the move or copy constructor.)

[AdmiralCurtiss]

I'm surprised this works even though there's no constructor, just struct initialization.

[AdmiralCurtiss]

Ah, because it doesn't actually work, only on MSVC. Or maybe only on C++20?

[AdmiralCurtiss]

gcc accepts it in C++20 mode, so I guess that's a C++20 feature.

[Dentomologist]

There's only one caller that checks the return value, and it will always be packs.back() from there, so this probably isn't necessary.

On the other hand I think Manager.cpp could use a lot of refactoring beyond the scope of this PR, and a clean solution would probably depend on that, so go ahead and do whatever seems best.

[AdmiralCurtiss]

I think I'll leave this as-is.

[Miksel12]

I made this issue report quite some time ago which might be relevant: https://bugs.dolphin-emu.org/issues/11768

[iwubcode]

@Miksel12 - that's a bit off topic but at least part of that was resolved in #8360 , however, I closed that as I fully plan to kill off ResourcePacks in the near future..

[AdmiralCurtiss]

@iwubcode Not sure what your plans are but I think the idea of a resource pack is pretty good, just the implementation is botched. If we can rejigger it (or make a new format, probably less confusing) so that Dolphin can read from the pack directly instead of having to 'install' and 'uninstall' them by actually extracting/deleting files this could be a significant improvement over the pile of texture files we currently have. I suspect that was the plan in the first place due to the no-compression requirement, but something went wrong somewhere along the way -- maybe shouldn't have exposed the fact that it's just a zip file, because that gives people the idea that you eg. can't pack it further in a 7z for distribution.

[iwubcode]

@AdmiralCurtiss - yeah. This is going off topic to your PR, sorry. I'm all for a different approach (ex: read from zip) but if you want to change the format, definitely ask the content creators. Things have stayed the same for years, which is why I finally decided to start moving on with my idea.

My biggest issue with the ResourcePacks was the install/uninstall. I think we agree that you can't really do that when the files contained are multiple GBs. Even ignoring the unresponsive UI right now, it's incredibly slow on Windows.

From a texture pack creator perspective, I heard a couple comments that they didn't see much gains with the 'zip' format, which is why they wanted '7z' or something with more compression. Then there's the fact that while testing, any sort of compression isn't really very viable either. So do you support something different during the 'creation' stage compared with the release stage? The dump/test process for texture pack creation needs an overhauled imo so that's possible but that's another topic..

The format aside, we can look at the UI. The arrows, are really klunky and need to be replaced with drag-drop at least (I'm not a UI person but it's hard to work with). Additionally, in scenarios where you have MANY packs, the fact all packs across all games are bunched together makes them difficult to manage. There may be a tiered approach you can leverage but it gets potentially klunky (see other UIs for attempts at tiered approaches). I think a per-game view makes more sense.

The actual metadata is fine. At least I don't have anything else to say on it right now :)

My plan was to migrate texture packs (likely as they are) into the graphics mod system. I was thinking of renaming this to something else to encompass graphics-mods, texture packs, and dynamic input textures in one view. This already supports drag/drop for ordering and is game-centric. Might change some things to support the additional types of data. I was hoping to read from the texture packs as they were so users with packs wouldn't need to do any conversion. I'd just default the metadata if it wasn't present.

[sepalani]

Otherwise, code LGTM, untested.

[dolphin-emu-bot]

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

• nddemo-bumpmapping on sw-lin-mesa: diff

_{^{automated-fifoci-reporter}}