GGGS shader: discard by per-band NoData (not v<=0) + Nearest data sampling (#122)#133
Merged
Conversation
Replace shader's hardcoded v<=0.0 discard with per-tile NoData uniform
The GGGS fragment shader discarded every sample with v<=0.0, a convention valid only for the depth/sidescan band 1 (the mosaicker floors real returns to >=1 and reserves 0 for NoData). After #108 enabled arbitrary band selection, bands with legitimately zero or negative valid samples (uncertainty, quality, signed offsets) were mis-rendered: valid <=0 samples were silently discarded and the colormap range was computed over the positive subset only. Add u_has_nodata (int, for GLSL 1.20 portability) and u_nodata (float) uniforms; replace the discard with 'if(u_has_nodata != 0 && v == u_nodata) discard'. The uniforms are set per-tile inside the render loop (after texture->bind(0)) from tile->hasNoData()/tile->noData(); tiles without NoData discard nothing. Asymmetry note: u_min/u_max stay layer-global (set once outside the loop from data_min_/data_max_) because the colormap range is shared across tiles, while the NoData uniform is per-tile because NoData can differ per tile/band. This asymmetry is intentional. Remove the stale [camp#108] LIMITATION comment that referenced this follow-up. Tests: add a Float32 GggsTile range test (negative valid samples + non-zero NoData) that RUNS, and an offscreen render test pinning the discard path that SKIPS when offscreen GL is unavailable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…onsistent NoData compare
…aN + float-compare comments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The GGGS raster fragment shader discarded every sample with
v <= 0.0and auto-rangedover the rest — correct for depth band 1 (mosaicker floors returns to
>= 1, reserves 0for NoData), but wrong for the other bands the #108 picker now exposes (uncertainty,
quality, signed offsets): valid zero/negative samples were silently discarded and the
colormap range computed only over positives.
This makes the shader honor the band's actual NoData value instead of a hardcoded
sign test. The CPU side already re-queried per-band NoData (
GggsTile::loadPixels); thiscloses the gap on the GPU side.
Closes #122.
Changes
gggs_tile_layer.cpp): addedu_has_nodata(int) +u_nodata(float)uniforms; replaced
if(v <= 0.0) discard;withif(u_has_nodata != 0 && v == u_nodata) discard;. Uniforms set per-tile inside the render loop (NoData can differ pertile/band);
u_min/u_maxstay layer-global (shared colormap range). Removed the stale[camp#108] LIMITATIONcomment that pointed here.gggs_tile.cpp): the R32F data texture min/mag filter switchedLinear → Nearest. For real survey data viewed for quality assessment, faithful
cell-accurate values matter more than smoothness — interpolation fabricates values not in
the data (and produced a bright one-texel halo around NoData regions, since interpolated
boundary texels never equal the sentinel). The colormap LUT texture stays Linear (it's
a color ramp, not data). On-screen blit smoothing (
SmoothPixmapTransform) for datalayers is a separate, operator-selectable follow-up: Operator-selectable interpolation for data layers (Nearest default; A/B on real data) #132.
gggs_tile.cpp): the CPU range exclusion nowcompares in float to match the shader's float discard.
Tests
./ui_ws/test.sh camp→ 124 tests, 0 failures, 4 skipped (the skips are offscreen-GLrender tests that SKIP-not-FAIL where no GL is available):
test_gggs_tile.cpp—NegativeValidSamplesNonZeroNoData(Float32 tile, NoData=9999,samples
[-3, 0, -1.5, 9999]): range covers the negatives, NoData excluded. Runs.test_gggs_render.cpp—NoDataDiscardHonorsUniform: asserts a valid0.0region rendersopaque (only the new code does; the old
v<=0discards it) and the NoData block renderstransparent. Designed to fail if the discard is reverted to
v<=0. GL-gated (SKIPsin-container; a GL host confirms fail-on-revert).
Review
Driven via
/run-issue; 3 rounds of pre-pushreview-code(Round 1 caught that the rendertest didn't discriminate the fix). Final: approved, 0 must-fix, 0 suggestions.
Follow-up
remaining
SmoothPixmapTransformdata-blit smoothing on real data.Authored-By:
Claude Code AgentModel:
Claude Opus 4.8 (1M context)