Skip to content

Review: maketx --alpha1-detect#153

Merged
lgritz merged 1 commit intoAcademySoftwareFoundation:masterfrom
lgritz:lg-maketx
Aug 30, 2011
Merged

Review: maketx --alpha1-detect#153
lgritz merged 1 commit intoAcademySoftwareFoundation:masterfrom
lgritz:lg-maketx

Conversation

@lgritz
Copy link
Copy Markdown
Collaborator

@lgritz lgritz commented Aug 30, 2011

maketx --alpha1-detect will omit alpha from textures whose input images had designated alpha channels that were 1.0 for all pixels. Somewhat similar in operation to --monochrome-detect and --constant-detect.

Also adds ImageBufAlgo::isConstantChannel() utility and does some minor cleanup/simplification to isConstantColor.

(Yes, this is because we have textures with pointless alpha=1 everywhere, and it's easier to fix in image-to-texture conversion than it is to actually fix the problem upstream.)

@lgritz
Copy link
Copy Markdown
Collaborator Author

lgritz commented Aug 30, 2011

Also note that I put the test for alpha=1 before the "is it monochrome" test. So it will correctly collapse CCC1 textures to C, if you know what I mean, whereas previously CCC1 textures would not only keep the pointless alpha, they also would fail to recognize the texture as monochrome (except for the very rare case of C==1).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the refactor of this loop, much cleaner. Are there any issues with src.getpixel if the src image is null or doesnt have any pixels? (what does getpixel do?)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

src is a ref, not a pointer, so it can't be null. I'll put in a check to immediately return 'true' if there are no valid pixels at all.

@jeremyselan
Copy link
Copy Markdown
Contributor

Looks good to me. I'm fine with this name (maketx --alpha1-detect) or any other one.

@fpsunflower
Copy link
Copy Markdown
Contributor

LGTM

+1 for Solomon's suggestion of --opaque-detect though

…es had designated alpha channels that were 1.0 for all pixels. Also adds ImageBufAlgo::isConstantChannel() utility.
@lgritz lgritz merged commit 6caea93 into AcademySoftwareFoundation:master Aug 30, 2011
@jeremyselan
Copy link
Copy Markdown
Contributor

Someday we may want to tag what the original alpha value was in the metadata. (Particularly if we ever introduce an option to drop alphas with a constant 0.0 value). But until we cross that line, I'm all for not adding extra complexity until needed.

@lgritz
Copy link
Copy Markdown
Collaborator Author

lgritz commented Aug 30, 2011

At SPI, we checked our shader library and without exception, RGB textures are treated the same as RGBA textures where alpha=1, and I suspect that this is probably a widespread convention.

Dropping constant alpha that != 1 seems inherently dangerous, and hard to imagine that the extra logic in the texture system or shaders would be worth the storage savings, for which is surely a rare case of completely constant but non-1.0 alpha.

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.

3 participants