Add overdraw as option to GAPID UI framebuffer view #2070
Conversation
Are there any stencil formats greater than 8 bit? Let's assume there are - then GAPIC already has a tonemapper which does what you're after. That said, in my past experiences, stencil buffers are more commonly used for bit-masks than scalar values. Maybe we should offer up a way to show / hide individual bits in the UI? |
|
There are on Vulkan, and my understanding is that its generally 8 bits on GLES as well, but not required. Ok I'll look into the tonemapper, thanks. So here I'm trying to show specifically the overdraw information, which just happens to have a stencil format, not a general stencil view. I agree that that should have some sort of bitmask view. |
sean-purcell
changed the title
In that case, I think it makes sense to translate from |
|
Sure, sounds good. |
sean-purcell
force-pushed
the
gapic-overdraw
branch
from
89bff4c to
3381acb
Compare
sean-purcell
force-pushed
the
gapic-overdraw
branch
from
3381acb to
42150a8
Compare
|
Looks like this now: |
sean-purcell
changed the title
The histogram / HDR slider looks a bit empty - is the client handling the new count component? |
|
Once you have lots of overdraw going on, spotting the gradient changes might be challenging. |
Fixed, I forgot to update the channel for count image binning.
Hmm, good point, currently the only way to investigate minor differences is to adjust things with the histogram slider. It could be made more discoverable by maybe opening the histogram by default in overdraw mode? |
Could be a little in-your-face. I was thinking that maybe the default should be a tonemap range of 0-10, and you can expand this range if you wanted to. Not have this default interfere with the user's preferences might be tricky though (user adjusts limits, clicks to the next frame, gets annoyed that the limits are reset) |
Ok, that sounds reasonable.
The limit resetting is the current behaviour with depth and colour images, is it annoying that it gets reset there as well? |
It can be yes. |
Maybe that behaviour should be changed in general then, e.g. have ImagePanel.ImageComponent store preferences for each image type and use those instead of histogram.getInitialRange if available when the image is changed. |
|
However, the logic for a non-annoying experience is not simple.
I'm not sure what the perfect solution is here (if there is one). |
|
That's true. At least in Vulkan, one potentially reasonable option is to maintain the settings as long as they're switching around within a single subpass, as those are all rendering to the same framebuffer attachments. |
| @@ -276,6 +305,9 @@ public static Format from(Image.Format format) { | |||
| boolean color = isColorFormat(format); | |||
| int channels = getChannelCount(format, color ? COLOR_CHANNELS : DEPTH_CHANNELS); | |||
| boolean is8bit = are8BitsEnough(format, color ? COLOR_CHANNELS : DEPTH_CHANNELS); | |||
| if (isCountFormat(format) && is8bit) { | |||
There was a problem hiding this comment.
is8bit is still computed by only looking at the color or depth channels
| /** | ||
| * @return true if this image should be tone-mapped when displayed. | ||
| */ | ||
| public boolean isCount(); |
There was a problem hiding this comment.
Combine isHDR and isCount into an enum, since there really are only 3 possibilities:
LDR, HDR and Count.
| @@ -80,7 +88,7 @@ public Range getInitialRange(double snapThreshold) { | |||
| * @param high if true, return the upper limit on the percentile's bin, otherwise the lower limit. | |||
| * @return the absolute pixel value at the specified percentile in the histogram. | |||
| */ | |||
| private double getPercentile(int percentile, boolean high) { | |||
| public double getPercentile(int percentile, boolean high) { | |||
| /** | ||
| * An {@link ArrayImage} that represents an 8bit count image. | ||
| */ | ||
| public static class Count8Image extends Luminance8Image { |
There was a problem hiding this comment.
Don't extend Luminance8Image, you are overriding practically all methods anyways.
| @@ -428,7 +433,7 @@ public PixelInfo getInfo() { | |||
| return info; | |||
| } | |||
|
|
|||
| private static class Pixel implements PixelValue { | |||
| protected static class Pixel implements PixelValue { | |||
| return true; | ||
| } | ||
|
|
||
| protected static class Pixel implements PixelValue { |
| private final int count; | ||
|
|
||
| public Pixel(byte count) { | ||
| this.count = count & 0xFF; |
There was a problem hiding this comment.
Use UnsignedBytes.toInt instead of & 0xFF for readability.
| @@ -723,7 +789,7 @@ public double getAlphaMax() { | |||
|
|
|||
| @Override | |||
| public boolean isNormalized() { | |||
| return true; | |||
| return normalized; | |||
There was a problem hiding this comment.
sigh... this really just duplicated isHDR() (well, the inverse, but still...). Feel free to leave as is, or incorporate with the change of isHDR and isCount into an enum as I'm requesting.
- Combine isHDR and isCount into a single enum, as they are mutually exclusive - Other minor fixes
|
@pmuetschard @ben-clayton PTAL |
Previously the feature was only accessible through
gapit screenshot. Looks like:Two issues:
The rescaling is somewhat hacky as currently done (specifically stencil images that set the normalized flag get rescaled), the goal is to take the stencil bytes and scale them from [0, max] to [0, 255], while also storing the original values of the data to show at the bottom when mousing over the overdraw image (so that the maximum overdrawn area of the screen is shown as white, but the actual value can be queried). @pmuetschard @ben-clayton @AWoloszyn What would be the best way to architect this in gapic? Thanks
It's currently using the depth buffer icon, I would appreciate some guidance on how to generate a good icon for this.