Skip to content
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

Add Scene._pickVoxelCoordinate to report voxel tile and sample numbers #11784

Merged
merged 12 commits into from
Jan 29, 2024

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented Jan 23, 2024

Description

This is another incremental step towards Voxel Picking.

A new pass type FrameState.Passes.pickVoxel renders an ID for the picked voxel tile and an index of the picked sample within the tile. This pass is triggered, and the rendered ID reported, by the new Scene.pickVoxel.

Example usage:

  const pickedPrimitive = scene.pick(mousePosition);
  if (!Cesium.defined(pickedPrimitive)) {
    return;
  }
  if (pickedPrimitive.primitive instanceof Cesium.VoxelPrimitive) {
    const voxelCoord = scene.pickVoxel(mousePosition);
    const tileIndex = 256 * voxelCoord[0] + voxelCoord[1];
    const sampleIndex = 256 * voxelCoord[2] + voxelCoord[3];
    pickReport.innerHTML = `Tile ${tileIndex}, Sample ${sampleIndex}`;
  }

image

Testing plan

See this testing Sandcastle to report tile and sample indices for random procedural voxels.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@ggetz
Copy link
Contributor

ggetz commented Jan 24, 2024

Thanks for the PR @jjhembd!

Before we get into the implementation details, I wanted to check on the API. In your example, it looks like you are making two pick calls: One general scene.pick to determine the primitive type, and then one call to scene.pickVoxel to get the voxel coordinate.

Is the first call strictly necessary? Is is OK to call scene.pickVoxel in isolation?

@jjhembd
Copy link
Contributor Author

jjhembd commented Jan 24, 2024

scene.pickVoxel currently returns tile and sample indices, but nothing to indicate which voxel dataset those indices correspond to. The full sequence would look like this:

const voxelPrimitive = scene.pick(mousePosition);
const voxelCoordinate = scene.pickVoxel(mousePosition);
const voxelData = voxelPrimitive.getData(voxelCoordinate); // This method not implemented yet

We could potentially combine all 3 calls into one scene.pickVoxel method that returns everything (or undefined if there is no VoxelPrimitive at the cursor position).

@ggetz
Copy link
Contributor

ggetz commented Jan 24, 2024

We could potentially combine all 3 calls into one scene.pickVoxel method that returns everything (or undefined if there is no VoxelPrimitive at the cursor position).

I think that would be simplest from a public API perspective.

@jjhembd
Copy link
Contributor Author

jjhembd commented Jan 24, 2024

I moved the coordinate picking method to a private method. We can clean up the public API in a subsequent PR.
I also tried looking up the data from CPU memory, based on the rendered tile and sample numbers. This exposed a couple bugs which are now fixed. Here is the Sandcastle demonstrating data lookup.

Remaining tasks for this PR:

  • Add spec for new Scene._pickVoxelCoordinate method
  • Address TODO items in comments

@jjhembd jjhembd changed the title Add Scene.pickVoxel to report voxel tile and sample numbers Add Scene._pickVoxel to report voxel tile and sample numbers Jan 25, 2024
@jjhembd jjhembd changed the title Add Scene._pickVoxel to report voxel tile and sample numbers Add Scene._pickVoxelCoordinate to report voxel tile and sample numbers Jan 26, 2024
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Overall this is looking good to me @jjhembd!

Everything is working well on the procedural dataset. Looking forward to see how its working with any collected data.

packages/engine/Source/Scene/PickFramebuffer.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Picking.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Picking.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Picking.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/Scene.js Outdated Show resolved Hide resolved
@ggetz
Copy link
Contributor

ggetz commented Jan 29, 2024

@jjhembd Updates are looking good to me! Anything else in this PR before merging?

@jjhembd
Copy link
Contributor Author

jjhembd commented Jan 29, 2024

@ggetz I think this one is ready to go. The remaining changes will go in the 3rd PR.
Thanks for your reviews!

@ggetz ggetz merged commit 2b59f05 into voxel-picking Jan 29, 2024
9 checks passed
@ggetz ggetz deleted the voxel-pickbuffer branch January 29, 2024 17:14
@jjhembd jjhembd mentioned this pull request Feb 13, 2024
6 tasks
@lilleyse
Copy link
Contributor

@jjhembd it looks like frameState.passes.pick and frameState.passes.pickVoxel are mutually exclusive but it might simplify the code if they are both true.

  frameState.passes.pick = true;
  frameState.passes.pickVoxel = true;

That way the code can leverage the existing picking infrastructure and only check for frameState.passes.pickVoxel in VoxelPrimitive.

@javagl and I were discussing this in the context of #12075 for a new flag frameState.passes.pickMetadata.

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.

3 participants