Skip to content
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

HiresTextures: Also look for directories with 3-character IDs #3456

Merged
merged 1 commit into from
Jan 6, 2016

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Jan 6, 2016

People who make texture packs usually release them using a specific ID (for instance SX4E01). Users who have a different version of the game (like the PAL version SX4P01) then need to rename the custom texture folder to match. This is a lot simpler than renaming every texture file, as was required with the old texture format, but it's still something that users can forget to do. To make that unnecessary, this change makes it possible to use three-character region-free IDs for custom texture folders, similarly to how game INIs can use three-character IDs. Once most people have updated to Dolphin versions that include this change (hopefully 5.0!), those who make texture packs will be able to name them with three-character IDs, removing the need for users to rename anything.

@AdmiralCurtiss
Copy link
Contributor

If I'm seeing this right, this only reads from a region-agnostic directory when a region-specific one doesn't exist.

Would it make more sense to check each individual file in both places when both directories exist, so you can have an agnostic directory for most textures and then region-specific ones for overrides or textures that only exist in one version of the game?

@JosJuice
Copy link
Member Author

JosJuice commented Jan 6, 2016

I don't really see an advantage in putting textures that only are in specific versions in a separate folder. Textures that aren't used by the user's version of the game won't cause any problems (unless there's a hash collision, but that's rare, and the same problem applies to all other textures), and they are usually few enough that they won't have a noticeable impact on prefetching. Also, it would be more complicated for me to implement :P

On the other hand, not using the three-character folder at all when there's a longer one could be useful for Triforce games and unlicensed games that share their first three characters with licensed GC/Wii software that have a three-character texture folder. (Though this probably won't be much of a problem in reality, considering that there currently aren't any Triforce texture packs and most ID collisions are between very similar games like F-Zero GX and F-Zero AX...)

@AdmiralCurtiss
Copy link
Contributor

Fair enough.

@@ -86,11 +86,14 @@ void HiresTexture::Update()
s_textureCache.clear();
}

const std::string& gameCode = SConfig::GetInstance().m_strUniqueID;
const std::string& game_id = SConfig::GetInstance().m_strUniqueID;
std::string texture_directory = File::GetUserPath(D_HIRESTEXTURES_IDX) + game_id;

This comment was marked as off-topic.

This comment was marked as off-topic.

@degasus
Copy link
Member

degasus commented Jan 6, 2016

Sounds like a very worth improvement. LGTM

People who make texture packs usually release them using a specific ID
(for instance SX4E01). Users who have a different version of the game
(like the PAL version SX4P01) then need to rename the custom texture
folder to match. This is a lot simpler than renaming every texture file,
as was required with the old texture format, but it's still something
that users can forget to do. To make that unnecessary, this change makes
it possible to use three-character region-free IDs for custom texture
folders, similarly to how game INIs can use three-character IDs. Once
most people have updated to Dolphin versions that include this change,
those who make texture packs will be able to name them with
three-character IDs, removing the need for users to rename anything.
@delroth
Copy link
Member

delroth commented Jan 6, 2016

@JosJuice: This comment grants you the permission to merge this pull request whenever you think it is ready. After addressing the remaining comments, click this link to merge.


LGTM. @dolphin-emu-bot allowmerge

JosJuice added a commit that referenced this pull request Jan 6, 2016
HiresTextures: Also look for directories with 3-character IDs
@JosJuice JosJuice merged commit 42237dc into dolphin-emu:master Jan 6, 2016
@JosJuice JosJuice deleted the custom-texture-all-regions branch January 6, 2016 13:51
@BigheadSMZ
Copy link

This is nice, it will prevent a lot of "why is it not working" questions in the long run. I do have a suggestion. Would it also be possible to expand on this idea and only check the first 6/3 characters of the folder path? This could allow additional text after the Game ID to enter the game title such as "SX4 - Xenoblade Chronicles", or maybe a style like "SX4 [Xenoblade Chronicles]". We can obviously derive the title from the ID by looking it up or checking it in Dolphin, so this would be cosmetic only, providing a quick reference to what game the pack is for.

Maybe this could get complicated if multiple folders exist, So if it's more trouble than what it's worth, kindly ignore this.

@JosJuice
Copy link
Member Author

JosJuice commented Jan 6, 2016

@BigheadSMZ That sounds useful, but I'm not going to work on it right now. Could you make a feature request on the issue tracker? https://bugs.dolphin-emu.org/projects/emulator

@BigheadSMZ
Copy link

Done. Not sure if its the best description, but I think anyone who's interested in it will get the idea.
https://bugs.dolphin-emu.org/issues/9230

@lioncash
Copy link
Member

lioncash commented Jan 6, 2016

@JosJuice This isn't what I meant by move it to a single function. Now there's a disjoint in where the texture path is handled. Now it's half in the update function and half in the new function. For example a nicer way of amending this is:

std::string HiresTexture::GetTextureFolder(const std::string& game_id)
{
    std::string texture_directory = File::GetUserPath(D_HIRESTEXTURES_IDX) + game_id;

    // If there's no directory with the region-specific ID, look for a 3-character region-free one
     if (!File::Exists(texture_directory))
       ... etc handling here

    return texture_directory;
}

the update function shouldn't have to care about dealing with the texture path at all.

@JosJuice
Copy link
Member Author

JosJuice commented Jan 6, 2016

@lioncash Fixed in PR #3459.

@dolphin-emu-bot
Copy link
Contributor

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

  • aeon-charge-attack on ogl-lin-nouveau: diff
  • chibi-robo-zfighting on ogl-lin-radeon: diff
  • medabots-crash on ogl-lin-radeon: diff
  • monkeyball-fuse on ogl-lin-radeon: diff
  • mp3-bloom on ogl-lin-radeon: diff
  • rs2-skybox on ogl-lin-radeon: diff
  • rs2-zfreeze on ogl-lin-radeon: diff
  • sf-assault-flashing on ogl-lin-radeon: diff
  • sms-bubbles on ogl-lin-radeon: diff
  • sw3-dt on ogl-lin-radeon: diff

automated-fifoci-reporter

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

Successfully merging this pull request may close these issues.

7 participants