New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement resource packs #7600

Open
wants to merge 1 commit into
base: master
from

Conversation

6 participants
@spycrab
Contributor

spycrab commented Dec 1, 2018

Enables users to more easily manage texture packs (and maybe some other stuff in the future).

Screenshot:
Screenshot

Read more about the format here.

@spycrab spycrab force-pushed the spycrab:resource_pack branch 4 times, most recently from 2c7de16 to 360d1a2 Dec 1, 2018

@spycrab spycrab force-pushed the spycrab:resource_pack branch 2 times, most recently from 3820e2d to 99c541f Dec 2, 2018

QMessageBox::critical(
this, tr("Error"),
tr("Invalid Pack %1 provided: %2").arg(QString::fromStdString(pack.GetPath())).arg(QString::fromStdString(pack.GetError())));
return;

This comment has been minimized.

@BhaaLseN

BhaaLseN Dec 2, 2018

Member

Would we want to show all invalid packs here, or really just the first one?

Show resolved Hide resolved Source/Core/UICommon/CMakeLists.txt Outdated
return packs;
}
std::vector<ResourcePack*> GetLowerPacks(ResourcePack& pack)

This comment has been minimized.

@BhaaLseN

BhaaLseN Dec 2, 2018

Member

This feels like there is an appropriate <algorithm> member in the STL (also for GetHigherPacks), but I can't seem to find it...might be wrong as well.

Maybe std::copy(pack, packs.end(), list)?

Why do you need to return pointers anyways if you have references already?

This comment has been minimized.

@spycrab

spycrab Dec 2, 2018

Contributor

Yeah, they don't have to be pointers.

std::copy will well...create copies as far as I'm aware. That's not what I want.

This comment has been minimized.

@BhaaLseN

BhaaLseN Dec 2, 2018

Member

Might not be std::copy, but I'm pretty sure there was something that gave you a range/slice delimited by two items.
Can probably ignore this unless someone else points out the right thing.

This comment has been minimized.

@Warepire

Warepire Dec 2, 2018

Are you looking for std::lower_bound() and std::upper_bound()? They'll simplify the expressions a bit, but they will just give you iterators, like std::find. You can also use std::distance() to pre-allocate the vector size. Should make this more efficient for a large amount of packs.

unzGetCurrentFileInfo(file, &texture_info, &filename[0], sizeof(filename), nullptr, 0, nullptr,
0);
if (filename.compare(0, 9, "textures/") != 0 || texture_info.uncompressed_size == 0)

This comment has been minimized.

@BhaaLseN

BhaaLseN Dec 2, 2018

Member

Should we be case-insensitive here?

This comment has been minimized.

@spycrab

spycrab Dec 2, 2018

Contributor

IMO: No.

This comment has been minimized.

@Zexaron

Zexaron Dec 3, 2018

Contributor

Depends on many circumstances, but over 50% of the time I capitalize my folders I create manually, I probably would have capitalized it in this case, but whatever, just how's someone suppose to know that on the outside.

Show resolved Hide resolved Source/Core/UICommon/ResourcePack/ResourcePack.cpp
Show resolved Hide resolved Source/Core/UICommon/ResourcePack/ResourcePack.cpp Outdated
Show resolved Hide resolved Source/Core/UICommon/ResourcePack/ResourcePack.cpp Outdated
Show resolved Hide resolved Source/Core/UICommon/UICommon.vcxproj.filters Outdated
@riking

UI design review:

  • I feel like there should be some way to describe what game(s) the pack is supposed to apply to. Whether that's autodetected (collect list of game IDs, xref against your library) or manual (field in the JSON specifying the game title) is up to you.
  • Is there a way to read the full pack name / description, no truncation, without opening the zip file? The current table-based UI seems inadequate to do that.
    While a detail pop-up is probably a pain to implement (another dialog box!) it may be the best way to do that.
  • I don't see any way to actually open the website link in your browser. Consider a double-click action, or if something prevents that, change it to a Copy Link (to clipboard) button without showing the website in the dialog (people can inspect it when they paste).

As always, these are suggestions, feel free to push back against any of them.

@spycrab spycrab force-pushed the spycrab:resource_pack branch 3 times, most recently from e78863b to 488b317 Dec 3, 2018

@spycrab

This comment has been minimized.

Contributor

spycrab commented Dec 3, 2018

I feel like there should be some way to describe what game(s) the pack is supposed to apply to. Whether that's autodetected (collect list of game IDs, xref against your library) or manual (field in the JSON specifying the game title) is up to you.

This adds more complexity to an already large PR, so I'm gonna say no to that one.

Is there a way to read the full pack name / description, no truncation, without opening the zip file? The current table-based UI seems inadequate to do that.
While a detail pop-up is probably a pain to implement (another dialog box!) it may be the best way to do that.

Fixed. I think it's sufficient for now.

I don't see any way to actually open the website link in your browser. Consider a double-click action, or if something prevents that, change it to a Copy Link (to clipboard) button without showing the website in the dialog (people can inspect it when they paste).

Links can be opened now by double clicking on them

@spycrab spycrab force-pushed the spycrab:resource_pack branch 2 times, most recently from 76c1ba8 to a90b76f Dec 3, 2018

@spycrab

This comment has been minimized.

Contributor

spycrab commented Dec 3, 2018

Fixed that files > 64K couldn't be extracted.

@spycrab spycrab force-pushed the spycrab:resource_pack branch from a90b76f to 0c5055e Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment