GH-5560 fix issues with handling slim skins

peterix · peterix · commit 9352c7336c4a · 2025-01-15T02:41:10.000+01:00
This actually dumbs down detection of skins that would be broken
as 'Classic' skins, but maybe we can do that better later.
diff --git a/launcher/skins/SkinTypes.cpp b/launcher/skins/SkinTypes.cpp
@@ -18,15 +18,7 @@ namespace Skins {
 SkinEntry::SkinEntry(const QString& name, const QString& path, const QImage& image, const QString& textureID, const QByteArray data) : name(name), filename(path)
 {
     internal = false;
-    if (qAlpha(image.pixel(54, 20)) == 0)
-    {
-        // slim-only textures will have this pixel fully transparent
-        slimVariant = SkinData{data, image, textureID};
-    }
-    else
-    {
-        classicVariant = SkinData{data, image, textureID};
-    }
+    fileVariant = SkinData{data, image, textureID};
 }
 
 SkinEntry::SkinEntry(const QString& name, const QString& pathSlim, const QString& pathClassic): name(name)
@@ -56,73 +48,59 @@ SkinEntry::SkinEntry(const QString& name, const QString& pathSlim, const QString
 
 const QImage& SkinEntry::getTextureFor(Model model) const
 {
-    switch(model)
+    if(internal)
     {
-        case Model::Slim:
+        switch(model)
         {
-            if(slimVariant)
-            {
+            case Model::Slim:
                 return slimVariant->texture;
-            }
-        }
-        break;
-        case Model::Classic:
-        {
-            if(classicVariant)
-            {
+            case Model::Classic:
                 return classicVariant->texture;
-            }
         }
-        break;
+    }
+
+    if(fileVariant)
+    {
+        return fileVariant->texture;
     }
     return nullImage;
 }
 
 QString Skins::SkinEntry::getTextureIDFor(Model model) const
 {
-    switch(model)
+    if(internal)
     {
-        case Model::Slim:
+        switch(model)
         {
-            if(slimVariant)
-            {
+            case Model::Slim:
                 return slimVariant->textureID;
-            }
-        }
-        break;
-        case Model::Classic:
-        {
-            if(classicVariant)
-            {
+            case Model::Classic:
                 return classicVariant->textureID;
-            }
         }
-        break;
+    }
+
+    if(fileVariant)
+    {
+        return fileVariant->textureID;
     }
     return QString();
 }
 
 QByteArray Skins::SkinEntry::getTextureDataFor(Model model) const
 {
-    switch(model)
+    if(internal)
     {
-        case Model::Slim:
+        switch(model)
         {
-            if(slimVariant)
-            {
+            case Model::Slim:
                 return slimVariant->data;
-            }
-        }
-        break;
-        case Model::Classic:
-        {
-            if(classicVariant)
-            {
+            case Model::Classic:
                 return classicVariant->data;
-            }
         }
-        break;
     }
+
+    if(fileVariant)
+        return fileVariant->data;
     return QByteArray();
 }
 
@@ -251,46 +229,38 @@ const QImage & SkinData::getListTexture(Model model) const
 
 const QImage & SkinEntry::getListTexture() const
 {
-    if(slimVariant)
+    if(internal)
     {
         return slimVariant->getListTexture(Model::Slim);
     }
-    else if(classicVariant)
+
+    if(fileVariant)
     {
-        return classicVariant->getListTexture(Model::Classic);
+        return fileVariant->getListTexture(Model::Classic);
     }
     return nullImage;
 }
 
-bool SkinEntry::hasModel(Model model) const
+bool SkinEntry::matchesId(const QString& textureID) const
 {
-    switch(model)
+    if(internal)
     {
-        case Model::Slim:
+        if(slimVariant && slimVariant->textureID == textureID)
         {
-            return !!slimVariant;
+            return true;
         }
-        break;
-        case Model::Classic:
+        if(classicVariant && classicVariant->textureID == textureID)
         {
-            return !!classicVariant;
+            return true;
         }
-        break;
+        return false;
     }
-    return false;
-}
 
-nonstd::optional<Model> SkinEntry::matchesId(const QString& textureID) const
-{
-    if(slimVariant && slimVariant->textureID == textureID)
-    {
-        return Model::Slim;
-    }
-    if(classicVariant && classicVariant->textureID == textureID)
+    if(fileVariant && fileVariant->textureID == textureID)
     {
-        return Model::Classic;
+        return true;
     }
-    return nonstd::nullopt;
+    return false;
 }
 
 }
diff --git a/launcher/skins/SkinTypes.h b/launcher/skins/SkinTypes.h
@@ -40,23 +40,25 @@ struct SkinEntry
     SkinEntry(const QString& name, const QString& pathSlim, const QString& pathClassic);
     SkinEntry() {};
 
-    nonstd::optional<Model> matchesId(const QString& textureID) const;
+    bool matchesId(const QString& textureID) const;
     bool isNull() const
     {
-        return !slimVariant && !classicVariant;
+        if(internal)
+            return !slimVariant && !classicVariant;
+        return !fileVariant;
     }
 
     const QImage& getListTexture() const;
     const QImage& getTextureFor(Model model) const;
     QString getTextureIDFor(Model model) const;
     QByteArray getTextureDataFor(Model model) const;
-    bool hasModel(Model model) const;
 
     bool internal = false;
     QString name;
     QString filename;
     nonstd::optional<SkinData> slimVariant;
     nonstd::optional<SkinData> classicVariant;
+    nonstd::optional<SkinData> fileVariant;
 };
 
 struct CapeEntry
diff --git a/launcher/skins/SkinsModel.cpp b/launcher/skins/SkinsModel.cpp
@@ -470,7 +470,7 @@ const Skins::SkinEntry & SkinsModel::skinEntryByTextureID(const QString& texture
 {
     for(const auto& entry: m_skins)
     {
-        if(entry.matchesId(textureID) != nonstd::nullopt)
+        if(entry.matchesId(textureID))
         {
             return entry;
         }
diff --git a/launcher/ui/dialogs/AccountsDialog.cpp b/launcher/ui/dialogs/AccountsDialog.cpp
@@ -219,7 +219,6 @@ void AccountsDialog::revertEdits()
     m_skinEdit = nonstd::nullopt;
     ui->btnApplyChanges->setEnabled(false);
     ui->btnResetChanges->setEnabled(false);
-    updateModelToMatchSkin();
     updateSkinDisplay();
 }
 
@@ -285,7 +284,6 @@ void AccountsDialog::editSkin(const Skins::SkinEntry& newEntry)
         return;
 
     m_skinEdit->skinEntry = newEntry;
-    updateModelToMatchSkin();
     updateSkinDisplay();
 }
 
@@ -442,44 +440,9 @@ void AccountsDialog::updateStates()
         (!maybeEntry.isNull()) ? maybeEntry : Skins::SkinEntry("player", "", image, textureID, playerSkinData)
     };
 
-    updateModelToMatchSkin();
     updateSkinDisplay();
 }
 
-void AccountsDialog::updateModelToMatchSkin()
-{
-    auto& skinEntry = effectiveSkin();
-    auto model = effectiveModel();
-    bool hasClassic = skinEntry.hasModel(Skins::Model::Classic);
-    bool hasSlim = skinEntry.hasModel(Skins::Model::Slim);
-
-    if(m_skinEdit)
-    {
-        if(model == Skins::Model::Classic && !hasClassic)
-        {
-            m_skinEdit->model = Skins::Model::Slim;
-            ui->radioSlim->setChecked(true);
-        }
-        if(model == Skins::Model::Slim && !hasSlim)
-        {
-            m_skinEdit->model = Skins::Model::Classic;
-            ui->radioClassic->setChecked(true);
-        }
-    }
-    else
-    {
-        if(model == Skins::Model::Classic && !hasClassic)
-        {
-            ui->radioSlim->setChecked(true);
-        }
-        if(model == Skins::Model::Slim && !hasSlim)
-        {
-            ui->radioClassic->setChecked(true);
-        }
-    }
-}
-
-
 void AccountsDialog::updateSkinDisplay()
 {
     QImage capeImage;
@@ -492,10 +455,6 @@ void AccountsDialog::updateSkinDisplay()
     auto& skin = effectiveSkin();
     auto model = effectiveModel();
 
-    bool hasClassic = skin.hasModel(Skins::Model::Classic);
-    bool hasSlim = skin.hasModel(Skins::Model::Slim);
-    ui->radioClassic->setEnabled(hasClassic);
-    ui->radioSlim->setEnabled(hasSlim);
     auto textureID = skin.getTextureIDFor(model);
     ui->saveSkinButton->setEnabled(m_skinsModel->skinEntryByTextureID(textureID).isNull());
 

Original file line number	Diff line number	Diff line change
`@@ -18,15 +18,7 @@ namespace Skins {`
`18`	`18`	`SkinEntry::SkinEntry(const QString& name, const QString& path, const QImage& image, const QString& textureID, const QByteArray data) : name(name), filename(path)`
`19`	`19`	`{`
`20`	`20`	`internal = false;`
`21`		`- if (qAlpha(image.pixel(54, 20)) == 0)`
`22`		`- {`
`23`		`- // slim-only textures will have this pixel fully transparent`
`24`		`- slimVariant = SkinData{data, image, textureID};`
`25`		`- }`
`26`		`- else`
`27`		`- {`
`28`		`- classicVariant = SkinData{data, image, textureID};`
`29`		`- }`
	`21`	`+ fileVariant = SkinData{data, image, textureID};`
`30`	`22`	`}`
`31`	`23`
`32`	`24`	`SkinEntry::SkinEntry(const QString& name, const QString& pathSlim, const QString& pathClassic): name(name)`
`@@ -56,73 +48,59 @@ SkinEntry::SkinEntry(const QString& name, const QString& pathSlim, const QString`
`56`	`48`
`57`	`49`	`const QImage& SkinEntry::getTextureFor(Model model) const`
`58`	`50`	`{`
`59`		`- switch(model)`
	`51`	`+ if(internal)`
`60`	`52`	`{`
`61`		`- case Model::Slim:`
	`53`	`+ switch(model)`
`62`	`54`	`{`
`63`		`- if(slimVariant)`
`64`		`- {`
	`55`	`+ case Model::Slim:`
`65`	`56`	`return slimVariant->texture;`
`66`		`- }`
`67`		`- }`
`68`		`- break;`
`69`		`- case Model::Classic:`
`70`		`- {`
`71`		`- if(classicVariant)`
`72`		`- {`
	`57`	`+ case Model::Classic:`
`73`	`58`	`return classicVariant->texture;`
`74`		`- }`
`75`	`59`	`}`
`76`		`- break;`
	`60`	`+ }`
	`61`	`+`
	`62`	`+ if(fileVariant)`
	`63`	`+ {`
	`64`	`+ return fileVariant->texture;`
`77`	`65`	`}`
`78`	`66`	`return nullImage;`
`79`	`67`	`}`
`80`	`68`
`81`	`69`	`QString Skins::SkinEntry::getTextureIDFor(Model model) const`
`82`	`70`	`{`
`83`		`- switch(model)`
	`71`	`+ if(internal)`
`84`	`72`	`{`
`85`		`- case Model::Slim:`
	`73`	`+ switch(model)`
`86`	`74`	`{`
`87`		`- if(slimVariant)`
`88`		`- {`
	`75`	`+ case Model::Slim:`
`89`	`76`	`return slimVariant->textureID;`
`90`		`- }`
`91`		`- }`
`92`		`- break;`
`93`		`- case Model::Classic:`
`94`		`- {`
`95`		`- if(classicVariant)`
`96`		`- {`
	`77`	`+ case Model::Classic:`
`97`	`78`	`return classicVariant->textureID;`
`98`		`- }`
`99`	`79`	`}`
`100`		`- break;`
	`80`	`+ }`
	`81`	`+`
	`82`	`+ if(fileVariant)`
	`83`	`+ {`
	`84`	`+ return fileVariant->textureID;`
`101`	`85`	`}`
`102`	`86`	`return QString();`
`103`	`87`	`}`
`104`	`88`
`105`	`89`	`QByteArray Skins::SkinEntry::getTextureDataFor(Model model) const`
`106`	`90`	`{`
`107`		`- switch(model)`
	`91`	`+ if(internal)`
`108`	`92`	`{`
`109`		`- case Model::Slim:`
	`93`	`+ switch(model)`
`110`	`94`	`{`
`111`		`- if(slimVariant)`
`112`		`- {`
	`95`	`+ case Model::Slim:`
`113`	`96`	`return slimVariant->data;`
`114`		`- }`
`115`		`- }`
`116`		`- break;`
`117`		`- case Model::Classic:`
`118`		`- {`
`119`		`- if(classicVariant)`
`120`		`- {`
	`97`	`+ case Model::Classic:`
`121`	`98`	`return classicVariant->data;`
`122`		`- }`
`123`	`99`	`}`
`124`		`- break;`
`125`	`100`	`}`
	`101`	`+`
	`102`	`+ if(fileVariant)`
	`103`	`+ return fileVariant->data;`
`126`	`104`	`return QByteArray();`
`127`	`105`	`}`
`128`	`106`
`@@ -251,46 +229,38 @@ const QImage & SkinData::getListTexture(Model model) const`
`251`	`229`
`252`	`230`	`const QImage & SkinEntry::getListTexture() const`
`253`	`231`	`{`
`254`		`- if(slimVariant)`
	`232`	`+ if(internal)`
`255`	`233`	`{`
`256`	`234`	`return slimVariant->getListTexture(Model::Slim);`
`257`	`235`	`}`
`258`		`- else if(classicVariant)`
	`236`	`+`
	`237`	`+ if(fileVariant)`
`259`	`238`	`{`
`260`		`- return classicVariant->getListTexture(Model::Classic);`
	`239`	`+ return fileVariant->getListTexture(Model::Classic);`
`261`	`240`	`}`
`262`	`241`	`return nullImage;`
`263`	`242`	`}`
`264`	`243`
`265`		`-bool SkinEntry::hasModel(Model model) const`
	`244`	`+bool SkinEntry::matchesId(const QString& textureID) const`
`266`	`245`	`{`
`267`		`- switch(model)`
	`246`	`+ if(internal)`
`268`	`247`	`{`
`269`		`- case Model::Slim:`
	`248`	`+ if(slimVariant && slimVariant->textureID == textureID)`
`270`	`249`	`{`
`271`		`- return !!slimVariant;`
	`250`	`+ return true;`
`272`	`251`	`}`
`273`		`- break;`
`274`		`- case Model::Classic:`
	`252`	`+ if(classicVariant && classicVariant->textureID == textureID)`
`275`	`253`	`{`
`276`		`- return !!classicVariant;`
	`254`	`+ return true;`
`277`	`255`	`}`
`278`		`- break;`
	`256`	`+ return false;`
`279`	`257`	`}`
`280`		`- return false;`
`281`		`-}`
`282`	`258`
`283`		`-nonstd::optional<Model> SkinEntry::matchesId(const QString& textureID) const`
`284`		`-{`
`285`		`- if(slimVariant && slimVariant->textureID == textureID)`
`286`		`- {`
`287`		`- return Model::Slim;`
`288`		`- }`
`289`		`- if(classicVariant && classicVariant->textureID == textureID)`
	`259`	`+ if(fileVariant && fileVariant->textureID == textureID)`
`290`	`260`	`{`
`291`		`- return Model::Classic;`
	`261`	`+ return true;`
`292`	`262`	`}`
`293`		`- return nonstd::nullopt;`
	`263`	`+ return false;`
`294`	`264`	`}`
`295`	`265`
`296`	`266`	`}`
Original file line number	Diff line number	Diff line change
`@@ -470,7 +470,7 @@ const Skins::SkinEntry & SkinsModel::skinEntryByTextureID(const QString& texture`
`470`	`470`	`{`
`471`	`471`	`for(const auto& entry: m_skins)`
`472`	`472`	`{`
`473`		`- if(entry.matchesId(textureID) != nonstd::nullopt)`
	`473`	`+ if(entry.matchesId(textureID))`
`474`	`474`	`{`
`475`	`475`	`return entry;`
`476`	`476`	`}`