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

Add transparency toggle #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Jampi0n
Copy link

@Jampi0n Jampi0n commented Jun 22, 2022

@@ -328,7 +332,8 @@ def settings(self):
mobase.PluginSetting("background r", self.__tr("Red channel of background colour"), 0),
mobase.PluginSetting("background g", self.__tr("Green channel of background colour"), 0),
mobase.PluginSetting("background b", self.__tr("Blue channel of background colour"), 0),
mobase.PluginSetting("background a", self.__tr("Alpha channel of background colour"), 0)]
mobase.PluginSetting("background a", self.__tr("Alpha channel of background colour"), 0),
mobase.PluginSetting("transparency", self.__tr("If enabled, transparency will be displayed."), True)]
Copy link
Member

Choose a reason for hiding this comment

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

It'd be more technically accurate to refer to it as alpha rather than transparency or translucency. The whole reason this PR is necessary is that it's not always transparency data.

@@ -151,9 +151,11 @@
glVersionProfile.setVersion(2, 1)

class DDSWidget(QOpenGLWidget):
def __init__(self, ddsFile, debugContext = False, parent = None, f = Qt.WindowFlags()):
def __init__(self, ddsPreview, ddsFile, debugContext = False, parent = None, f = Qt.WindowFlags()):
Copy link
Member

Choose a reason for hiding this comment

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

I'm not keen on creating a cyclic dependency between DDSWidget and DDSPreview. Technically, it's possible to find uses for a DDS widget other than the preview, so we might want to move it out of this plugin into a library and make the plugin depend on that library. You can break the cycle by creating a class that holds the current options which both the widget and preview plugin have a reference to an instance of.

@AnyOldName3
Copy link
Member

It might be worth considering whether it's sensible to treat the alpha channel as something special here. In most image editors that expect to deal with textures, you can toggle channels individually, see specific ones together, and things like that. As an example, here's the manual for Blender's image editor: https://docs.blender.org/manual/en/latest/editors/image/introduction.html#header. Scroll to the Display Channels subheading (it won't let me link it directly). It lets you do extra stuff like view the alpha channel as greyscale, and that's likely to be useful, too.

@Jampi0n
Copy link
Author

Jampi0n commented Jun 27, 2022

I have added a drop down menu to select which channels to display. I used the same options as Blender's image editor.

The single color channels can look quite bad due to compression in the textures, so that's something to keep in mind when testing them.

Images

Drop down menu:
image

Tool tip on hover:
image

Gif of different channels:
Preview

BC7 quick compression:
image

BC7 slow compression:
image

@Jampi0n
Copy link
Author

Jampi0n commented Jun 30, 2022

This is my first time doing shader stuff, but it seems to work.
I also separated the channel presets (RGBA, RGB, A, R, G, and B) from the shader/channel logic. This means the shaders are capable of performing any channel transformation and it's easy to add new channel presets.

Edit: forgot to update the translation file, but I'll wait until the code is acceptable to avoid unnecessary changes

Comment on lines 183 to 204

def flattenList(lst: Iterable):
tmp = []
for e in lst:
if isinstance(e, Iterable):
tmp += flattenList(e)
elif isinstance(e, (int, float)):
tmp += [float(e)]
else:
raise ValueError("Can only set a matrix with numbers.")
return tmp

if isinstance(matrix, Iterable):
flattened = flattenList(matrix)
if len(flattened) != 16:
raise ValueError("Must provide exactly 16 values.")
matrix = QMatrix4x4(flattened)

if isinstance(matrix, QMatrix4x4):
self.channelMatrix = matrix
else:
raise TypeError(str(channels) + " is not a valid ColourChannels object.")
raise ValueError("Can only set a matrix with numbers.")
Copy link
Member

Choose a reason for hiding this comment

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

Can we not just do self.channelMatrix = QMatrix4x4(matrix) and let PyQt6/SIP handle any conversions? I don't think this lets everything that can be converted to a matrix through, and the extra supported stuff and error reporting probably aren't that useful. It's more obvious if it just accepts what people would expect it to accept, which is matrices.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that''s much better. Don't know why I made it so complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants