-
Notifications
You must be signed in to change notification settings - Fork 17
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
Display mipmap preview levels as tabs instead of using a spinner #80
Conversation
connect(saveAsLink, &QLinkLabel::activated, fTexture, &QTextureBox::saveAs); | ||
fTexture->setTexture(tex); | ||
auto levelTabs = new QTabWidget(this); | ||
for (size_t level = 0; level < tex->getNumLevels(); level++) { |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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? :/
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
There was a problem hiding this comment.
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.
Great QoL improvement. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may not be too important, but this can also be done by replacing the spin box with QTabBar
that still lazy loads the image data, instead of loading all mips at once. It would simplify the diff, as well as avoid populating several mostly-identical widgets with only a different image content.
1ff4602
to
95cbffd
Compare
The |
This improves a part of the mipmap preview window that I found confusing at first - the "Level" spinner at the top:
When I first used this preview window, I assumed that the level was a property stored in the mipmap object. I expected that changing the spinner would modify the PRP file, so I never touched it. I didn't realize that the spinner has no permanent effect and actually lets you preview other mipmap levels beyond the first one.
This PR replaces the spinner with a tab bar, which IMO communicates the meaning and behavior more clearly. As a bonus, the tab titles also display each level's pixel width/height.