Skip to content

Points now only sample and are simplified#694

Merged
TheJeran merged 2 commits into
mainfrom
jp/samplePoints
Jul 20, 2026
Merged

Points now only sample and are simplified#694
TheJeran merged 2 commits into
mainfrom
jp/samplePoints

Conversation

@TheJeran

@TheJeran TheJeran commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

This reworks the points to just sample the textures and not require the extra textureData stored in the global store. That hasn't been removed yet as it is tied to the texture creation and I will do that in a new PR.

Changes

  • vertexIdx: float --> Int floats cant capture large ints
  • Calculate texCoord implicitly from vertexIdx for sampling
  • Calculate position implicitly from texCoord using GLOBAL_SCALE definition
  • Moved the flipping of values for reprojection to the plot level
  • Pass just the aspect ratio as a uniform to handle the change in aspect ratio with new CRS
WG84 Mollweide
browzarr-plot (6) browzarr-plot (5)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the PointCloud component and its vertex shader to optimize rendering by sampling values directly from a 3D texture array instead of passing them as vertex attributes, alongside simplifying coordinate calculations and clipping checks. The review feedback identifies several critical issues: a typo in the aspect ratio uniform calculation (shape.x/shape.x), a potential division-by-zero risk in ProjectionTexture.ts due to missing safety checks, a potential NaN value for globalscale in 2D mode, and an incorrect 32-bit integer limit representation (2e31 - 1). Additionally, it is recommended to pass the aspect ratio as a uniform rather than a preprocessor define to prevent slow shader recompilations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/components/plots/PointCloud.tsx Outdated
);
uniforms.fillValue.value = fillValue?? NaN
uniforms.maskValue.value = maskValue
uniforms.aspect.value = shape.x/shape.x;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

There is a typo here: shape.x/shape.x will always evaluate to 1.0. It should be shape.x/shape.y to correctly update the aspect ratio uniform.

Suggested change
uniforms.aspect.value = shape.x/shape.x;
uniforms.aspect.value = shape.x/shape.y;

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.

oops this isnt even supposed to be there still

Comment on lines +214 to +215
const u = (px - minX) / xDiff;
const v = (py - minY) / yDiff;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Removing the safety checks for xDiff > 0 and yDiff > 0 introduces a risk of division by zero (resulting in NaN or Infinity values) if the dataset has a single point or a single line of longitude/latitude. We should restore these checks.

Suggested change
const u = (px - minX) / xDiff;
const v = (py - minY) / yDiff;
const u = xDiff > 0 ? (px - minX) / xDiff : 0;
const v = yDiff > 0 ? (py - minY) / yDiff : 0;

const targetHeight = (remapTexture && remapTexture.image) ? remapTexture.image.height : height;
const is2D = dataShape.length === 2;
}, [dataShape]);
const globalscale = dataShape[2]/500

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If dataShape has fewer than 3 elements (e.g., in 2D mode), dataShape[2] will be undefined, resulting in globalscale being NaN. Adding a safe fallback prevents potential rendering issues.

Suggested change
const globalscale = dataShape[2]/500
const globalscale = (dataShape[2] ?? 500) / 500

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.

Worry about that if/when 2D is added

Comment thread src/components/plots/PointCloud.tsx
Comment thread src/components/plots/PointCloud.tsx

uniform sampler2D maskTexture;
uniform sampler3D map[12];
uniform vec3 textureDepths;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Declare uniform float aspect; to receive the aspect ratio uniform instead of relying on the ASPECT_RATIO preprocessor define.

uniform vec3 textureDepths;
uniform float aspect;

Comment on lines +51 to +53
float px = (texCoord.x - 0.5);
float py = (texCoord.y - 0.5) / ASPECT_RATIO;
float pz = (texCoord.z - 0.5) * timeScale;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Use the aspect uniform instead of the ASPECT_RATIO define to avoid shader recompilation when the aspect ratio changes.

    float px = (texCoord.x - 0.5);
    float py =  (texCoord.y - 0.5) / aspect;
    float pz = (texCoord.z - 0.5) * timeScale;

@lazarusA

lazarusA commented Jul 20, 2026

Copy link
Copy Markdown
Member

Why sampling when you can plot the real thing? Or what does it mean sampling in this context?

Edit: Oh I see, is actual sampling. Please let's have this as optional, when performance might be an issue. I really would prefer to display the real points.

Comment on lines +59 to +72
float sample1(vec3 p, int index) { // Shader doesn't support dynamic indexing so we gotta use switching
if (index == 0) return texture(map[0], p).r;
else if (index == 1) return texture(map[1], p).r;
else if (index == 2) return texture(map[2], p).r;
else if (index == 3) return texture(map[3], p).r;
else if (index == 4) return texture(map[4], p).r;
else if (index == 5) return texture(map[5], p).r;
else if (index == 6) return texture(map[6], p).r;
else if (index == 7) return texture(map[7], p).r;
else if (index == 8) return texture(map[8], p).r;
else if (index == 9) return texture(map[9], p).r;
else if (index == 10) return texture(map[10], p).r;
else if (index == 11) return texture(map[11], p).r;
else return 0.0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this looks awfully inconvenient.

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.

This is how it is in literally every shader.

@TheJeran

Copy link
Copy Markdown
Collaborator Author

2 reasons. First is you implemented sampling in the last PR so since that workflow is already here might as well just adopt it for the non-projected data.
Second, this frees up the textureData that was being stored along with the textures in the globalStore. So this will cut memory usage for textures in half.

Sampling means instead of writing the value directly to the vertex we just use the index to sample from the data in the texture.
We can also write the values directly to the vertex. But the end result is the same except now we cut memory usage in half.

#435 is the issue with the duplicate data. There should be some way of using the data already in the textures to store on the vertices themselves to avoid the duplicate data. But when i looked into it, it was a headache I didn't have the energy to try to solve. But if you can figure out some way of mapping the split up data in the textures to vertices that would be the best solution.

@TheJeran
TheJeran merged commit 3072557 into main Jul 20, 2026
6 checks passed
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