-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Use eye coordinates for voxel raymarching #12933
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
Conversation
Thank you for the pull request, @jjhembd! ✅ We can confirm we have a CLA on file for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jjhembd, I took a quick pass trying out the sandcastles.
The only issue I see is for cylinder procedural tileset, there seems to be an unexpected visual effect as I move the cursor. You cannot really see my cursor in the video, but the small flash of a tiny voxel cluster is following the cursor, and their is also a station cluster flashing on and off in the upper right next next to the voxel inspector. The same effect is visible on the main
, so this may not be related to the changes in this PR. The picking behavior looks good on the other 5 examples.
Screencast.from.2025-09-30.13-16-43.mp4
edit I actually see this in Ellipsoid procedural tileset as well. It seems slightly more noticeable after zooming in or out.
Screencast.from.2025-09-30.13-28-01.mp4
float clipAmount; | ||
float pixelWidth = czm_metersPerPixel(position); | ||
bool breakAndDiscard = false; | ||
for (int i = 0; i < ${clippingPlanesLength}; ++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning this up! Should this (either here or a later PR) be adjusted to use compiler define
statements?
/** | ||
* An event triggered when a new clipping plane is added to the collection. Event handlers | ||
* are passed the new plane and the index at which it was added. | ||
* @type {Event} | ||
* @default Event() | ||
*/ | ||
this.planeAdded = new Event(); | ||
|
||
/** | ||
* An event triggered when a new clipping plane is removed from the collection. Event handlers | ||
* are passed the new plane and the index from which it was removed. | ||
* @type {Event} | ||
* @default Event() | ||
*/ | ||
this.planeRemoved = new Event(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should both be readonly, and therefore the default
should be extraneous.
/** | |
* An event triggered when a new clipping plane is added to the collection. Event handlers | |
* are passed the new plane and the index at which it was added. | |
* @type {Event} | |
* @default Event() | |
*/ | |
this.planeAdded = new Event(); | |
/** | |
* An event triggered when a new clipping plane is removed from the collection. Event handlers | |
* are passed the new plane and the index from which it was removed. | |
* @type {Event} | |
* @default Event() | |
*/ | |
this.planeRemoved = new Event(); | |
/** | |
* An event triggered when a new clipping plane is added to the collection. Event handlers | |
* are passed the new plane and the index at which it was added. | |
* @type {Event} | |
* @readonly | |
*/ | |
this.planeAdded = new Event(); | |
/** | |
* An event triggered when a new clipping plane is removed from the collection. Event handlers | |
* are passed the new plane and the index from which it was removed. | |
* @type {Event} | |
* @readonly | |
*/ | |
this.planeRemoved = new Event(); | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jjhembd! Overall, this is good shape. The code is looking pretty clean to me. I didn't look deeply into the math itself, but the results are looking great!
Finally a reminder to update CHANGES.md
.
} | ||
|
||
vec3 scaleShapeUvToShapeSpace(in vec3 shapeUv) { | ||
// TODO: scaling is wrong! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mentioned offline that this is TODO is on your radar–A reminder to address here or to open an issue.
vec3 rtu = u_cylinderEcToRadialTangentUp * positionEC; | ||
// 2. Compute change in angular and radial coordinates. TODO: scaling camera is wrong! | ||
vec2 dPolar = computePolarChange(rtu.xy, u_cameraShapePosition.x * u_cylinderWorldToLocalScale.x); | ||
// TODO: this is wrong! Scaling needs to be applied in local XYZ coordinates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
I did some basic testing on existing voxel sandcastles and everything I observed matches |
Description
This PR reworks the voxel shaders to perform the raymarching in eye-space coordinates, rather than model-space. This avoids precision problems when zoomed in close to small samples in a large model space.
Most of the precision problem came from the sampling of the voxel. The traversal in
Octree.glsl
has been re-worked to input a two-part coordinate:[x, y, z, w]
indices of the tile within the tileset[0, 1]
indicating thex, y, z
positions within the tileThis new input to the traversal effectively gives us nearly double precision when sampling a large dataset.
Some parts of the code are still in a local model-centered coordinate system:
The latter 2 items could also be converted to eye coordinates with additional work. I did not do that work in this PR, mostly because of time constraints. I also haven't observed significant precision issues with those intersections, so that work does not seem urgent at this time.
Issue number and link
Fixes #12061
Testing plan
main
.Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change