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.

Sign up for GitHub

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

Display mipmap preview levels as tabs instead of using a spinner #80

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Display mipmap preview levels as tabs instead of using a spinner #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 20 additions & 17 deletions src/PrpShop/PRP/Surface/QMipmap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <QGridLayout>
#include <QPainter>
#include <QSettings>
#include <QTabBar>
#include <QFileDialog>
#include <QMessageBox>
#include <QRegularExpression>
Expand Down Expand Up @@ -134,31 +135,33 @@ QMipmap_Preview::QMipmap_Preview(plCreatable* pCre, QWidget* parent)
fTexture = new QTextureBox(scroll);
scroll->setWidget(fTexture);

QWidget* levelWidget = new QWidget(this);
QGridLayout* levelLayout = new QGridLayout(levelWidget);
levelLayout->setContentsMargins(4, 4, 4, 4);
levelLayout->setHorizontalSpacing(8);
QSpinBox* levelSel = new QSpinBox(levelWidget);
if (tex->getCompressionType() == plBitmap::kJPEGCompression)
levelSel->setRange(0, 0);
else
levelSel->setRange(0, tex->getNumLevels() - 1);
connect(levelSel, QOverload<int>::of(&QSpinBox::valueChanged),
QTabBar* levelSel = new QTabBar(this);
for (size_t level = 0; level < tex->getNumLevels(); level++) {
Copy link
Member

Choose a reason for hiding this comment

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

Plasma has a weird thing where it doesn't actually store data for any levels where the width or height is 2px or smaller.

We can finally account for that here in PrpShop to hide tabs with invalid levels:

if ((tex->getLevelWidth(level) | tex->getLevelHeight(level)) & 0x03)
    continue;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, good to know. Is this always the case? I'm trying to make sense of the relevant Plasma code and it seems to be specific to DXT compression. It also reads like small sizes would have some data, just not in the usual format? :/

Copy link
Member

Choose a reason for hiding this comment

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

It definitely might be specific to DXT compression. I know that OpenGL rejected those smaller levels as invalid data when I was implementing texture loading for plGLPipeline, and there were enough places in Plasma doing tests against & 0x03 to make it apparent that something weird was going on with those.

We also had to workaround this in korman because of similar issues: https://github.com/H-uru/korman/blob/106ef015480fb3dc0ba3cef90491af042458d58b/korlib/texture.cpp#L80-L91

Copy link
Member

@Hoikas Hoikas Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Going off of memories of yester-year, the issue I most remember running into is Cyan's level size calculation for DXT5 compression being wrong. IIRC for a 2x2 mip level, it only allocates an alpha block but no color block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some quick research (read: punched "DXT" into Wikipedia). Apparently, DXT operates on blocks of 4x4 pixels, so it makes sense that it can't work for textures that are smaller than that along either axis.

I still can't really follow what the Plasma code is doing in that case. But based on some experimentation and staring at hex dumps of a couple of mipmaps, I think any levels narrower than 4 pixels are simply stored uncompressed.

This quick and dirty patch gives me reasonable results for e. g. Ahnonay's mipmaps. (Well, as reasonable as you can get for a texture that's 1 or 2 pixels wide, anyway. I agree with Deledrius' take in the comment that Hoikas linked.)

--- a/src/PrpShop/PRP/Surface/QMipmap.cpp
+++ b/src/PrpShop/PRP/Surface/QMipmap.cpp
@@ -87,7 +87,11 @@
             dp++;
         }
     }
-    fImage = new QImage(fImageData, tex->getLevelWidth(level),
+    const unsigned char* theActualImageData = fImageData;
+    if (tex->getCompressionType() == plMipmap::kDirectXCompression && (tex->getLevelWidth(level) < 4 || tex->getLevelHeight(level) < 4)) {
+        theActualImageData = static_cast<const unsigned char*>(tex->getLevelData(level));
+    }
+    fImage = new QImage(theActualImageData, tex->getLevelWidth(level),
                         tex->getLevelHeight(level), QImage::Format_ARGB32);
     resize(tex->getLevelWidth(level), tex->getLevelHeight(level));
     update();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be better to handle this case correctly in libHSPlasma. AFAICT, the mipmap levels in question do have image data stored, so we shouldn't hide them entirely. The special handling for the different image data format should go into DecompressImage and not here.

levelSel->insertTab(level, QString("%1x%2 (%3)")
.arg(tex->getLevelWidth(level))
.arg(tex->getLevelHeight(level))
.arg(level));
}
connect(levelSel, &QTabBar::currentChanged,
this, &QMipmap_Preview::setLevel);
levelLayout->addWidget(new QLabel(tr("Level:"), levelWidget), 0, 0);
levelLayout->addWidget(levelSel, 0, 1);
levelSel->setSizePolicy(QSizePolicy::MinimumExpanding, QSizePolicy::Minimum);
QLinkLabel* saveAsLink = new QLinkLabel(tr("Save As..."), levelWidget);
levelLayout->addWidget(saveAsLink, 1, 0, 1, 2);

QWidget* saveAsWidget = new QWidget(this);
QGridLayout* saveAsLayout = new QGridLayout(saveAsWidget);
saveAsLayout->setContentsMargins(4, 4, 4, 4);
saveAsLayout->setHorizontalSpacing(8);
QLinkLabel* saveAsLink = new QLinkLabel(tr("Save As..."), saveAsWidget);
saveAsLayout->addWidget(saveAsLink, 0, 0);
connect(fTexture, &QTextureBox::textureChanged, saveAsLink, &QWidget::setEnabled);
connect(saveAsLink, &QLinkLabel::activated, fTexture, &QTextureBox::saveAs);

fTexture->setTexture(tex);

QGridLayout* layout = new QGridLayout(this);
layout->setContentsMargins(0, 0, 0, 0);
layout->setVerticalSpacing(0);
layout->addWidget(levelWidget, 0, 0);
layout->addWidget(scroll, 1, 0);
layout->addWidget(levelSel, 0, 0);
layout->addWidget(saveAsWidget, 1, 0);
layout->addWidget(scroll, 2, 0);
}

void QMipmap_Preview::setLevel(int level)
Expand Down
1 change: 0 additions & 1 deletion src/PrpShop/PRP/Surface/QMipmap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

#include <PRP/Surface/plMipmap.h>
#include <QImage>
#include <QSpinBox>
#include "PRP/QObjLink.h"
#include "QBitmaskCheckBox.h"

Expand Down