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

Single channel should show as black and white in viewport #2952

Merged
merged 5 commits into from
Mar 21, 2023

Conversation

JGamache-autodesk
Copy link
Collaborator

A single channel image would show as red/black when used in a RGB context. Make sure those images show a black and white.

A single channel image would show as red/black when used in a RGB
context. Make sure those images show a black and white.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created test textures in 16/32 bit EXR with either Mono or Mono+Alpha channels to exercise all conversions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tests all possible Mono and Mono+Alpha precisions.

{
uniform token info:id = "ND_image_color3"
asset inputs:file = @textures/R8U.png@
color3f outputs:out
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even though the texture is single channel, we use it in a color3 context and expect black&white results.

Comment on lines 1501 to 1504
for (int y = 0; y < spec.height; y++) {
for (int x = 0; x < spec.width; x++) {
const int t = spec.width * y + x;
for (int b = 0; b < bpp_RGB32; b++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a time-critical cycle. Do we want to multithread it or optimize otherwise?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Texture loading is already done on a separate thread, so basically out of the critical path for MayaUSD. I can try to make sure the code is optimal though. A first look at the dissassembly showed that the compiler was unsure if the previous write did not overlap the read and was forced to re-read the source value.

vlasovi
vlasovi previously approved these changes Mar 21, 2023
Copy link
Collaborator

@vlasovi vlasovi left a comment

Choose a reason for hiding this comment

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

Looks good. The only thing is this change adds a texture format conversion cycle for some texture types, which might slow down a bit texture loading. I don't think this is a big concern, though.

Help the compiler by making sure that reading the source buffer is done
only once. Without that, the compiler has to consider that every read
could be on a buffer modified by the previous write (e.g. not allowed to know the buffers do
not overlap).
for (int b = 0; b < bpp_RGB32; b++) {
texels[t * bpp_RGB32 + b] = storage[t * bpp + (b % 4)];
for (int b = 0; b < bits_RGB32; b++) {
const unsigned char bits = storage[t * bpp + b];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Read the source buffer only once.

vlasovi
vlasovi previously approved these changes Mar 21, 2023
Prevents recomputing adresses and generates the most optimal assembly
code so far.
vlasovi
vlasovi previously approved these changes Mar 21, 2023
Copy link
Collaborator

@vlasovi vlasovi left a comment

Choose a reason for hiding this comment

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

Nice. This looks very good now.

vlasovi
vlasovi previously approved these changes Mar 21, 2023
Most likely to fail on mismatched variable sizes.
@JGamache-autodesk
Copy link
Collaborator Author

@seando-adsk ready for merge. Thanks!

@seando-adsk seando-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Mar 21, 2023
@seando-adsk seando-adsk merged commit 8a01717 into dev Mar 21, 2023
@seando-adsk seando-adsk deleted the gamaj/expand_monochrome_to_white branch March 21, 2023 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge vp2renderdelegate Related to VP2RenderDelegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants