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

Enable the FakeMipmapChange flag for US/EU Tactics Ogre, fixing replacement problem. #18001

Merged
merged 5 commits into from
Aug 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Common/Data/Format/IniFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,17 @@ std::map<std::string, std::string> Section::ToMap() const
return outMap;
}

std::vector<std::pair<std::string, std::string>> Section::ToVec() const {
std::vector<std::pair<std::string, std::string>> outVec;
for (std::vector<std::string>::const_iterator iter = lines.begin(); iter != lines.end(); ++iter)
{
std::string lineKey, lineValue;
if (ParseLine(*iter, &lineKey, &lineValue, NULL)) {
outVec.push_back(std::pair<std::string, std::string>(lineKey, lineValue));
}
}
return outVec;
}

bool Section::Delete(const char *key)
{
Expand Down
1 change: 1 addition & 0 deletions Common/Data/Format/IniFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class Section {
void Clear();

std::map<std::string, std::string> ToMap() const;
std::vector<std::pair<std::string, std::string>> ToVec() const; // Often more appropriate than ToMap() - doesn't artifically remove duplicates.

std::string *GetLine(const char* key, std::string* valueOut, std::string* commentOut);
const std::string *GetLine(const char* key, std::string* valueOut, std::string* commentOut) const;
Expand Down
17 changes: 13 additions & 4 deletions GPU/Common/TextureCacheCommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1114,8 +1114,8 @@ bool TextureCacheCommon::MatchFramebuffer(
return true;
}
} else {
WARN_LOG_ONCE(diffFormat2, G3D, "Ignoring possible texturing from framebuffer with incompatible format %s != %s at %08x (+%dx%d)",
GeTextureFormatToString(entry.format), GeBufferFormatToString(fb_format), fb_address, matchInfo->xOffset, matchInfo->yOffset);
WARN_LOG_ONCE(diffFormat2, G3D, "Ignoring possible texturing from framebuffer at %08x with incompatible format %s != %s (+%dx%d)",
fb_address, GeTextureFormatToString(entry.format), GeBufferFormatToString(fb_format), matchInfo->xOffset, matchInfo->yOffset);
return false;
}
}
Expand Down Expand Up @@ -2769,9 +2769,10 @@ bool TextureCacheCommon::PrepareBuildTexture(BuildTexturePlan &plan, TexCacheEnt
plan.scaleFactor = plan.scaleFactor > 4 ? 4 : (plan.scaleFactor > 2 ? 2 : 1);
}

bool isFakeMipmapChange = IsFakeMipmapChange();

bool isFakeMipmapChange = false;
if (plan.badMipSizes) {
isFakeMipmapChange = IsFakeMipmapChange();

// Check for pure 3D texture.
int tw = gstate.getTextureWidth(0);
int th = gstate.getTextureHeight(0);
Expand All @@ -2784,6 +2785,7 @@ bool TextureCacheCommon::PrepareBuildTexture(BuildTexturePlan &plan, TexCacheEnt
}
}
} else {
// We don't want to create a volume texture, if this is a "fake mipmap change".
pure3D = false;
}

Expand Down Expand Up @@ -2911,6 +2913,13 @@ bool TextureCacheCommon::PrepareBuildTexture(BuildTexturePlan &plan, TexCacheEnt
if (isFakeMipmapChange) {
// NOTE: Since the level is not part of the cache key, we assume it never changes.
plan.baseLevelSrc = std::max(0, gstate.getTexLevelOffset16() / 16);
// Tactics Ogre: If this is an odd level and it has the same texture address the below even level,
// let's just say it's the even level for the purposes of replacement.
// I assume this is done to avoid blending between levels accidentally?
// The Japanese version of Tactics Ogre uses multiple of these "double" levels to fit more characters.
if ((plan.baseLevelSrc & 1) && gstate.getTextureAddress(plan.baseLevelSrc) == gstate.getTextureAddress(plan.baseLevelSrc & ~1)) {
plan.baseLevelSrc &= ~1;
}
plan.levelsToCreate = 1;
plan.levelsToLoad = 1;
// Make sure we already decided not to do a 3D texture above.
Expand Down
36 changes: 28 additions & 8 deletions GPU/Common/TextureReplacer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -226,31 +226,50 @@ bool TextureReplacer::LoadIniValues(IniFile &ini, VFSBackend *dir, bool isOverri
ERROR_LOG(G3D, "Unsupported texture replacement version %d, trying anyway", version);
}

bool filenameWarning = false;
int badFileNameCount = 0;

std::map<ReplacementCacheKey, std::map<int, std::string>> filenameMap;
std::string badFilenames;

if (ini.HasSection("hashes")) {
auto hashes = ini.GetOrCreateSection("hashes")->ToMap();
auto hashes = ini.GetOrCreateSection("hashes")->ToVec();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will it make sense to map the same hash to two different filenames? The level is still part of the key, so this would just overwrite those duplicates silently anyway?

Not sure I understand what the intent would be for:

1234567890abcdef_0 = image1.png
1234567890abcdef_0 = image2.png
1234567890abcdef_0 = image3.png

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The map was deduplicating the empty keys which were confusing my logging, which I added because of a theory that turned out to be wrong.

// Format: hashname = filename.png
bool checkFilenames = g_Config.bSaveNewTextures && !g_Config.bIgnoreTextureFilenames && !vfsIsZip_;

for (const auto &item : hashes) {
ReplacementCacheKey key(0, 0);
int level = 0; // sscanf might fail to pluck the level, but that's ok, we default to 0. sscanf doesn't write to non-matched outputs.
// sscanf might fail to pluck the level if omitted from the line, but that's ok, we default level to 0.
// sscanf doesn't write to non-matched outputs.
int level = 0;
if (sscanf(item.first.c_str(), "%16llx%8x_%d", &key.cachekey, &key.hash, &level) >= 1) {
if (item.second.empty()) {
WARN_LOG(G3D, "Texture replacement: Ignoring hash mapping to empty filename: '%s ='", item.first.c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really an error, at least it used to be documented as an intentional way to disable saving a texture which you've decided it's not useful to upscale (example: solid color texture.) Seems annoying to logspam it.

-[Unknown]

continue;
}
filenameMap[key][level] = item.second;
if (checkFilenames) {
// TODO: We should check for the union of these on all platforms, really.
#if PPSSPP_PLATFORM(WINDOWS)
bool bad = item.second.find_first_of("\\ABCDEFGHIJKLMNOPQRSTUVWXYZ:<>|?*") != std::string::npos;
// Uppercase probably means the filenames don't match.
// Avoiding an actual check of the filenames to avoid performance impact.
filenameWarning = filenameWarning || item.second.find_first_of("\\ABCDEFGHIJKLMNOPQRSTUVWXYZ:<>|?*") != std::string::npos;
#else
filenameWarning = filenameWarning || item.second.find_first_of("\\:<>|?*") != std::string::npos;
bool bad = item.second.find_first_of("\\:<>|?*") != std::string::npos;
#endif
if (bad) {
badFileNameCount++;
if (badFileNameCount == 10) {
badFilenames.append("...");
} else if (badFileNameCount < 10) {
badFilenames.append(item.second);
badFilenames.push_back('\n');
}
}
}
} else if (item.first.empty()) {
INFO_LOG(G3D, "Ignoring [hashes] line with empty key: '= %s'", item.second.c_str());
} else {
ERROR_LOG(G3D, "Unsupported syntax under [hashes]: %s", item.first.c_str());
ERROR_LOG(G3D, "Unsupported syntax under [hashes], ignoring: %s = ", item.first.c_str());
}
}
}
Expand Down Expand Up @@ -308,9 +327,10 @@ bool TextureReplacer::LoadIniValues(IniFile &ini, VFSBackend *dir, bool isOverri
aliases_[pair.first] = alias;
}

if (filenameWarning) {
if (badFileNameCount > 0) {
auto err = GetI18NCategory(I18NCat::ERRORS);
g_OSD.Show(OSDType::MESSAGE_ERROR, err->T("textures.ini filenames may not be cross-platform (banned characters)"), 6.0f);
g_OSD.Show(OSDType::MESSAGE_WARNING, err->T("textures.ini filenames may not be cross - platform(banned characters)"), badFilenames, 6.0f);
WARN_LOG(G3D, "Potentially bad filenames: %s", badFilenames.c_str());
}

if (ini.HasSection("hashranges")) {
Expand Down
7 changes: 6 additions & 1 deletion assets/compat.ini
Original file line number Diff line number Diff line change
Expand Up @@ -265,11 +265,16 @@ ULJM06365 = true
# This hacks separates each mipmap to independent textures to display wrong-size mipmaps.
# For example this requires games like Tactics Ogre(Japanese) to display multi bytes fonts stored in mipmaps.
# See issue #5350.
# Tactics Ogre(Japanese)
# Tactics Ogre (Japanese)
ULJM05753 = true
NPJH50348 = true
ULJM06009 = true

# Tactics Ogre (US, EU). See #17491 / #17980
# Required for texture replacement to work correctly, though unlike JP, only one layer is actually used (two duplicates, really)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously just seems like a flaw in the texture replacement engine now. But I'm sure you're going to find all the games that use cube mipmaps and add this flag to each one. Good luck.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's not ideal. I will try to rectify this flaw, but I'm trying to get a release ready and the change I'm planning is a bit too big.

Though in reality we've only seen this in three or four games.

ULUS10565 = true
ULES01500 = true

[RequireBufferedRendering]
# Warn the user that the game will not work and have issue, if buffered rendering is not enabled.
# Midnight Club: LA Remix
Expand Down