From 9a24cf4e1046e2410ed14d0d014b156cbc36af70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Sat, 21 Oct 2023 21:11:44 +0200 Subject: [PATCH 1/3] Ensure placeholder skin always exists and has valid metrics Instead of adding the placeholder skin to the list of skins only when no skins have been loaded, always create the placeholder skin and use it only when no other skin is available. Use reasonable values for skin metrics of placeholder skin to improve its rendering. --- src/game/client/components/skins.cpp | 37 +++++++++++++++++----------- src/game/client/components/skins.h | 3 ++- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/game/client/components/skins.cpp b/src/game/client/components/skins.cpp index 43c6a9bde..4e5e1adaa 100644 --- a/src/game/client/components/skins.cpp +++ b/src/game/client/components/skins.cpp @@ -16,6 +16,26 @@ #include "skins.h" +CSkins::CSkins() : + m_PlaceholderSkin("dummy") +{ + m_PlaceholderSkin.m_OriginalSkin.Reset(); + m_PlaceholderSkin.m_ColorableSkin.Reset(); + m_PlaceholderSkin.m_BloodColor = ColorRGBA(1.0f, 1.0f, 1.0f, 1.0f); + m_PlaceholderSkin.m_Metrics.m_Body.m_Width = 64; + m_PlaceholderSkin.m_Metrics.m_Body.m_Height = 64; + m_PlaceholderSkin.m_Metrics.m_Body.m_OffsetX = 16; + m_PlaceholderSkin.m_Metrics.m_Body.m_OffsetY = 16; + m_PlaceholderSkin.m_Metrics.m_Body.m_MaxWidth = 96; + m_PlaceholderSkin.m_Metrics.m_Body.m_MaxHeight = 96; + m_PlaceholderSkin.m_Metrics.m_Feet.m_Width = 32; + m_PlaceholderSkin.m_Metrics.m_Feet.m_Height = 16; + m_PlaceholderSkin.m_Metrics.m_Feet.m_OffsetX = 16; + m_PlaceholderSkin.m_Metrics.m_Feet.m_OffsetY = 8; + m_PlaceholderSkin.m_Metrics.m_Feet.m_MaxWidth = 64; + m_PlaceholderSkin.m_Metrics.m_Feet.m_MaxHeight = 32; +} + bool CSkins::IsVanillaSkin(const char *pName) { return std::any_of(std::begin(VANILLA_SKINS), std::end(VANILLA_SKINS), [pName](const char *pVanillaSkin) { return str_comp(pName, pVanillaSkin) == 0; }); @@ -323,14 +343,6 @@ void CSkins::Refresh(TSkinLoadedCBFunc &&SkinLoadedFunc) SkinScanUser.m_pThis = this; SkinScanUser.m_SkinLoadedFunc = SkinLoadedFunc; Storage()->ListDirectory(IStorage::TYPE_ALL, "skins", SkinScan, &SkinScanUser); - if(m_Skins.empty()) - { - Console()->Print(IConsole::OUTPUT_LEVEL_STANDARD, "gameclient", "failed to load skins. folder='skins/'"); - CSkin DummySkin{"dummy"}; - DummySkin.m_BloodColor = ColorRGBA(1.0f, 1.0f, 1.0f); - auto &&pDummySkin = std::make_unique(std::move(DummySkin)); - m_Skins.insert({pDummySkin->GetName(), std::move(pDummySkin)}); - } } int CSkins::Num() @@ -344,15 +356,12 @@ const CSkin *CSkins::Find(const char *pName) if(pSkin == nullptr) { pSkin = FindOrNullptr("default"); - if(pSkin == nullptr) - return m_Skins.begin()->second.get(); - else - return pSkin; } - else + if(pSkin == nullptr) { - return pSkin; + pSkin = &m_PlaceholderSkin; } + return pSkin; } const CSkin *CSkins::FindOrNullptr(const char *pName, bool IgnorePrefix) diff --git a/src/game/client/components/skins.h b/src/game/client/components/skins.h index 67c37a586..72dfd689b 100644 --- a/src/game/client/components/skins.h +++ b/src/game/client/components/skins.h @@ -13,7 +13,7 @@ class CSkins : public CComponent { public: - CSkins() = default; + CSkins(); class CGetPngFile : public CHttpRequest { @@ -79,6 +79,7 @@ public: private: std::unordered_map> m_Skins; std::unordered_map> m_DownloadSkins; + CSkin m_PlaceholderSkin; size_t m_DownloadingSkins = 0; char m_aEventSkinPrefix[24]; From 4eb0ffd70123bbbbc0637d18d616a0fe0ef5cfbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Sat, 21 Oct 2023 21:50:51 +0200 Subject: [PATCH 2/3] Validate skin names when loading, refactor loading Prevent skins with invalid names from being loaded/downloaded. Improve log messages when skins cannot be loaded. Remove obsolete check for duplicate skins, as the storage handles duplicate files already. --- src/game/client/components/skins.cpp | 64 ++++++++++++++-------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/src/game/client/components/skins.cpp b/src/game/client/components/skins.cpp index 4e5e1adaa..7d910a33b 100644 --- a/src/game/client/components/skins.cpp +++ b/src/game/client/components/skins.cpp @@ -1,6 +1,7 @@ /* (c) Magnus Auvinen. See licence.txt in the root of the distribution for more information. */ /* If you are missing that file, acquire a complete release at teeworlds.com. */ +#include #include #include @@ -67,27 +68,30 @@ struct SSkinScanUser int CSkins::SkinScan(const char *pName, int IsDir, int DirType, void *pUser) { - auto *pUserReal = (SSkinScanUser *)pUser; + auto *pUserReal = static_cast(pUser); CSkins *pSelf = pUserReal->m_pThis; - if(IsDir || !str_endswith(pName, ".png")) + if(IsDir) return 0; - char aNameWithoutPng[128]; - str_copy(aNameWithoutPng, pName); - aNameWithoutPng[str_length(aNameWithoutPng) - 4] = 0; - - if(g_Config.m_ClVanillaSkinsOnly && !IsVanillaSkin(aNameWithoutPng)) + const char *pSuffix = str_endswith(pName, ".png"); + if(pSuffix == nullptr) return 0; - // Don't add duplicate skins (one from user's config directory, other from - // client itself) - if(pSelf->m_Skins.find(aNameWithoutPng) != pSelf->m_Skins.end()) + char aSkinName[IO_MAX_PATH_LENGTH]; + str_truncate(aSkinName, sizeof(aSkinName), pName, pSuffix - pName); + if(!CSkin::IsValidName(aSkinName)) + { + log_error("skins", "Skin name is not valid: %s", aSkinName); + return 0; + } + + if(g_Config.m_ClVanillaSkinsOnly && !IsVanillaSkin(aSkinName)) return 0; - char aBuf[IO_MAX_PATH_LENGTH]; - str_format(aBuf, sizeof(aBuf), "skins/%s", pName); - pSelf->LoadSkin(aNameWithoutPng, aBuf, DirType); + char aPath[IO_MAX_PATH_LENGTH]; + str_format(aPath, sizeof(aPath), "skins/%s", pName); + pSelf->LoadSkin(aSkinName, aPath, DirType); pUserReal->m_SkinLoadedFunc((int)pSelf->m_Skins.size()); return 0; } @@ -131,17 +135,15 @@ const CSkin *CSkins::LoadSkin(const char *pName, const char *pPath, int DirType) { CImageInfo Info; if(!LoadSkinPNG(Info, pName, pPath, DirType)) - return 0; + return nullptr; return LoadSkin(pName, Info); } bool CSkins::LoadSkinPNG(CImageInfo &Info, const char *pName, const char *pPath, int DirType) { - char aBuf[512]; if(!Graphics()->LoadPNG(&Info, pPath, DirType)) { - str_format(aBuf, sizeof(aBuf), "failed to load skin from %s", pName); - Console()->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "game", aBuf); + log_error("skins", "Failed to load skin PNG: %s", pName); return false; } return true; @@ -149,18 +151,14 @@ bool CSkins::LoadSkinPNG(CImageInfo &Info, const char *pName, const char *pPath, const CSkin *CSkins::LoadSkin(const char *pName, CImageInfo &Info) { - char aBuf[512]; - if(!Graphics()->CheckImageDivisibility(pName, Info, g_pData->m_aSprites[SPRITE_TEE_BODY].m_pSet->m_Gridx, g_pData->m_aSprites[SPRITE_TEE_BODY].m_pSet->m_Gridy, true)) { - str_format(aBuf, sizeof(aBuf), "skin failed image divisibility: %s", pName); - Console()->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "game", aBuf); + log_error("skins", "Skin failed image divisibility: %s", pName); return nullptr; } if(!Graphics()->IsImageFormatRGBA(pName, Info)) { - str_format(aBuf, sizeof(aBuf), "skin format is not RGBA: %s", pName); - Console()->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "game", aBuf); + log_error("skins", "Skin format is not RGBA: %s", pName); return nullptr; } @@ -297,11 +295,9 @@ const CSkin *CSkins::LoadSkin(const char *pName, CImageInfo &Info) Graphics()->FreePNG(&Info); - // set skin data if(g_Config.m_Debug) { - str_format(aBuf, sizeof(aBuf), "load skin %s", Skin.GetName()); - Console()->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "game", aBuf); + log_trace("skins", "Loaded skin %s", Skin.GetName()); } auto &&pSkin = std::make_unique(std::move(Skin)); @@ -366,22 +362,24 @@ const CSkin *CSkins::Find(const char *pName) const CSkin *CSkins::FindOrNullptr(const char *pName, bool IgnorePrefix) { - const char *pSkinPrefix = m_aEventSkinPrefix[0] ? m_aEventSkinPrefix : g_Config.m_ClSkinPrefix; if(g_Config.m_ClVanillaSkinsOnly && !IsVanillaSkin(pName)) { return nullptr; } - else if(pSkinPrefix && pSkinPrefix[0] && !IgnorePrefix) + + const char *pSkinPrefix = m_aEventSkinPrefix[0] != '\0' ? m_aEventSkinPrefix : g_Config.m_ClSkinPrefix; + if(!IgnorePrefix && pSkinPrefix[0] != '\0') { - char aBuf[24]; - str_format(aBuf, sizeof(aBuf), "%s_%s", pSkinPrefix, pName); + char aNameWithPrefix[48]; // Larger than skin name length to allow IsValidName to check if it's too long + str_format(aNameWithPrefix, sizeof(aNameWithPrefix), "%s_%s", pSkinPrefix, pName); // If we find something, use it, otherwise fall back to normal skins. - const auto *pResult = FindImpl(aBuf); + const auto *pResult = FindImpl(aNameWithPrefix); if(pResult != nullptr) { return pResult; } } + return FindImpl(pName); } @@ -397,7 +395,7 @@ const CSkin *CSkins::FindImpl(const char *pName) if(!g_Config.m_ClDownloadSkins) return nullptr; - if(str_find(pName, "/") != 0) + if(!CSkin::IsValidName(pName)) return nullptr; const auto SkinDownloadIt = m_DownloadSkins.find(pName); @@ -423,9 +421,9 @@ const CSkin *CSkins::FindImpl(const char *pName) CDownloadSkin Skin{pName}; - char aUrl[IO_MAX_PATH_LENGTH]; char aEscapedName[256]; EscapeUrl(aEscapedName, sizeof(aEscapedName), pName); + char aUrl[IO_MAX_PATH_LENGTH]; str_format(aUrl, sizeof(aUrl), "%s%s.png", g_Config.m_ClDownloadCommunitySkins != 0 ? g_Config.m_ClSkinCommunityDownloadUrl : g_Config.m_ClSkinDownloadUrl, aEscapedName); char aBuf[IO_MAX_PATH_LENGTH]; From 8adfb2f2ec93e983afb72a00535401ec32cefaf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Fri, 9 Feb 2024 21:43:43 +0100 Subject: [PATCH 3/3] Fix unlikely cases of skin blood color being wrong The check before calling `normalize` incorrectly excludes skins containing zero in any color component instead of excluding only skins with zero in all components. The check can be removed entirely, because it is already checked inside the `normalize` function whether the length of the `vec3` is zero, in which case a zero `vec3` will be returned. For very large skins which use large color values in at least one component, the `int` used for calculating the blood color could overflow. --- src/game/client/components/skins.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/game/client/components/skins.cpp b/src/game/client/components/skins.cpp index 7d910a33b..ad6728cde 100644 --- a/src/game/client/components/skins.cpp +++ b/src/game/client/components/skins.cpp @@ -207,8 +207,9 @@ const CSkin *CSkins::LoadSkin(const char *pName, CImageInfo &Info) // dig out blood color { - int aColors[3] = {0}; + int64_t aColors[3] = {0}; for(int y = 0; y < BodyHeight; y++) + { for(int x = 0; x < BodyWidth; x++) { uint8_t AlphaValue = pData[y * Pitch + x * PixelStep + 3]; @@ -219,10 +220,9 @@ const CSkin *CSkins::LoadSkin(const char *pName, CImageInfo &Info) aColors[2] += pData[y * Pitch + x * PixelStep + 2]; } } - if(aColors[0] != 0 && aColors[1] != 0 && aColors[2] != 0) - Skin.m_BloodColor = ColorRGBA(normalize(vec3(aColors[0], aColors[1], aColors[2]))); - else - Skin.m_BloodColor = ColorRGBA(0, 0, 0, 1); + } + + Skin.m_BloodColor = ColorRGBA(normalize(vec3(aColors[0], aColors[1], aColors[2]))); } CheckMetrics(Skin.m_Metrics.m_Body, pData, Pitch, 0, 0, BodyWidth, BodyHeight);