fixes all plot types reprojections#691
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces reprojection support across multiple visualization components (including PointCloud, Sphere, SphereBlocks, and CountryBorders) by integrating a remap texture and handling coordinate flipping. The review feedback highlights several critical issues, such as potential runtime crashes and division-by-zero errors when coordinate ranges or shape dimensions are zero. Additionally, it points out a Three.js-specific issue where setting a define to false still defines the macro in GLSL, recommending that these defines be conditionally omitted or deleted instead.
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.
… from data retrieval
…ssing dimensions in PointCloud component
|
Done with all suggestions 👍🏻 ! |
|
See issue #690 for the issue and example data. Also, I do like the deformed reprojection on the sphere. |
| defines: { | ||
| REPROJECT: remapTexture ? true : false, | ||
| FLIP_Y: flipY ? true : false, | ||
| IS_2D: is2D, |
There was a problem hiding this comment.
Is this, (and the subsequent shader counterpart) redundant? I don't think ou can have pointcloud with 2D data.
There was a problem hiding this comment.
on 2d point cloud data, we should support that. So, I will come back to that and enable it.
| newAxisDimArrays[yIdx] = yTicks; | ||
|
|
||
| const newAxisDimUnits = [...axisDimUnits]; | ||
| const targetUnits = (crsCheck.oProj as any)?.units || 'degrees'; |
There was a problem hiding this comment.
yes, but it needs something empty then. Because otherwise I think it will failed for some cases. Empty should be fine, ''.
| if (plotType === 'point-cloud') { | ||
| targetWidth = width; | ||
| targetHeight = height; | ||
| xTicks = linspace(minX, maxX, targetWidth); | ||
| yTicks = flipY ? linspace(maxY, minY, targetHeight) : linspace(minY, maxY, targetHeight); | ||
| data = new Uint16Array(targetWidth * targetHeight * 4); | ||
|
|
||
| const xDiff = Math.abs(maxX - minX); | ||
| const yDiff = Math.abs(maxY - minY); | ||
|
|
||
| for (let j = 0; j < targetHeight; j++) { | ||
| const lat = yArray[j]; | ||
| for (let i = 0; i < targetWidth; i++) { | ||
| const lon = xArray[i]; | ||
| const [px, py] = proj.forward([lon, lat]); | ||
| const valid = (isFinite(px) && isFinite(py)) ? 1 : 0; | ||
|
|
||
| const u = xDiff > 0 ? (px - minX) / xDiff : 0; | ||
| const v = yDiff > 0 ? (py - minY) / yDiff : 0; | ||
|
|
||
| const idx = (j * targetWidth + i) * 4; | ||
| data[idx] = THREE.DataUtils.toHalfFloat(u); | ||
| data[idx + 1] = THREE.DataUtils.toHalfFloat(v); | ||
| data[idx + 2] = THREE.DataUtils.toHalfFloat(valid); | ||
| } | ||
| } | ||
| } else { | ||
| targetWidth = Math.ceil(adjustedResolution * aspectRatio); | ||
| targetHeight = adjustedResolution; | ||
| xTicks = linspace(minX, maxX, targetWidth); | ||
| yTicks = flipY ? linspace(maxY, minY, targetHeight) : linspace(minY, maxY, targetHeight); | ||
|
|
||
| // Detect if coordinate axes are descending | ||
| const isXDescending = xArray.length > 1 ? xArray[0] > xArray[xArray.length - 1] : false; | ||
| const isYDescending = yArray.length > 1 ? yArray[0] > yArray[yArray.length - 1] : false; | ||
|
|
||
| data = new Uint16Array(targetWidth * targetHeight * 4); | ||
| const xRangeDiff = xMax - xMin; | ||
| const yRangeDiff = yMax - yMin; | ||
| for (let j = 0; j < targetHeight; j++) { | ||
| for (let i = 0; i < targetWidth; i++) { | ||
| const [lon, lat, valid] = safeInverse(proj, [xTicks[i], yTicks[j]]); | ||
| const u = xRangeDiff > 0 ? (isXDescending ? (xMax - lon) / xRangeDiff : (lon - xMin) / xRangeDiff) : 0; | ||
| const v = yRangeDiff > 0 ? (isYDescending ? (yMax - lat) / yRangeDiff : (lat - yMin) / yRangeDiff) : 0; | ||
|
|
||
| // Check boundary bounds to avoid displaying clamped blocks outside the dataset area | ||
| const inBounds = lon >= xMin && lon <= xMax && lat >= yMin && lat <= yMax; | ||
| const validVal = (valid === 1 && inBounds) ? 1 : 0; | ||
|
|
||
| const idx = (j * targetWidth + i) * 4; | ||
| data[idx] = THREE.DataUtils.toHalfFloat(u); | ||
| data[idx + 1] = THREE.DataUtils.toHalfFloat(v); | ||
| data[idx + 2] = THREE.DataUtils.toHalfFloat(validVal); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you explain the use cases where this is necessary? When does the normal case not work? I'm not convinced this isn't redundant or can't just be trivially undid by flipping the resulting texture.
There was a problem hiding this comment.
first part is to do reprojection directly on point data, second on your resampling texture strategy. Or? what else do you mean? Also, keeping track of flip or not flip is necessary.
There was a problem hiding this comment.
So the point-cloud doesn't increase in resolution?
There was a problem hiding this comment.
point-cloud should do native points. No need for resolution. What we see is what we get.
There was a problem hiding this comment.
Hmm... okay. I think I will come back to this. Something in my brain says there's some way to simplify it but I'm to tired to discover if thats true. But otherwise I'm content to merge.
There was a problem hiding this comment.
Let's open an issue to keep track of this then.
This reverts commit c07b0c4.
|
once you are done with your changes and satisfied let me know so that I can pull and test before merging. Thanks for taking a look so fast. |
…ss plot components
…h improved define handling
|
fallback to instead of
I agree that this does not apply to all projections, but if not information is provided about the |
|
The degrees thing is on the user to properly add the degrees in the proj string. But since all other units are massive. You can just add a check that if there are no units it checks if the new axis fits inside the lat/lon domains. if the max/min values are larger than 360 or less than -180 its almost certainly not going to be degrees. unless its a tiny square with lon/lat offsets. I also thing lon/lat projections have a name in the |
| const oldTextures = textures; | ||
| setTimeout(() => { | ||
| oldTextures.forEach((tex) => { | ||
| tex.dispose(); | ||
| if (tex.source) (tex.source as any).data = null; | ||
| }); | ||
| }, 0); | ||
| setTextures(null); |
There was a problem hiding this comment.
Can you reproduce this?
I really don't like adding extra code unless there's a specific reproducible error and I haven't had any issues with this since it's implementation
There was a problem hiding this comment.
yes, I can. I did test it. Without it, I will always see a red type error on console, I don't like those. This does work.
…ojectionTexture, user is responsible for it.








closes #690
Our test case goes from
to
A lot of the changes to other files were done to not rely on hard coded numbers and use a more generic approach to determine values. Plus some changes that were not done/updated when doing #674, yeah, mostly things missed in that PR. Plus, support for point-cloud and re-projections into the sphere plot type.
Also, analytics indeed doesn't work on reprojected data. We should address that in a different PR.
Some screenshots.
Sphere
FlatMap
Point clouds
Volume
Blocks