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

feat: change the number of depth samples in volume rendering to a controllable UI parameter #495

Merged
merged 41 commits into from
Nov 6, 2023

Conversation

seankmartin
Copy link
Contributor

Goal

Change the number of depth samples from a fixed 64 to a user-controlled variable. Named "samples per ray".

Primary changes

  1. Create a UI controllable int, default 64, min 1, max 8096 that defines the samples per ray.
  2. On update of samples per ray we trigger a redraw and a recompute of the chunk priority, in case this change switches the desired data resolution level. The data resolution level is currently based on the depth of the view frustum divided by the number of samples.
  3. Pass the samples per ray into both the chunk worker and the shader to accommodate this.
  4. Control the samples per ray in Python.

@google-cla
Copy link

google-cla bot commented Oct 10, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@seankmartin seankmartin changed the title Feat/variable ray samples feat: change the number of depth samples in volume rendering to a controllable UI parameter Oct 10, 2023
@jbms
Copy link
Collaborator

jbms commented Oct 11, 2023

This definitely seems like a good option, but what do you think about using the existing "Resolution" control (that currently does nothing) but modifying it to display the different resolutions in units of "voxel size / view frustum depth"? That way this "Resolution" control would behave similarly to the other Resolution controls and would no longer be a "display only" control.

@seankmartin
Copy link
Contributor Author

seankmartin commented Oct 11, 2023

I really like that idea. My hope was that the implementation here would be a stepping stone to something more robust, and that is a good suggestion.

We'd essentially be changing up the logic from the resolution being determined by a user setting of the near and far planes and samples per ray, to the number of samples per ray being determined by a user setting of the near and far planes and resolution level. Am I understanding that right?

Edit: Maybe we want to still leave the number of samples modifiable so that one can over/under sample the actual data resolution if they want to.

@jbms
Copy link
Collaborator

jbms commented Oct 12, 2023

To be clear, I was imagining that the actual user setting would still be number of samples, as you have it now, it would just be controlled using the (modified) "Resolution (3d)" control rather than a separate control. In order for that to make sense, the "Resolution (3d)" control would need to display the number of samples in log scale, and plot each resolution according to the number of samples that would be optimal for it, so that it displays in the correct position relative to the vertical bar that indicates the current "number of samples" selection.

@seankmartin
Copy link
Contributor Author

Sure, I understand better now - I still like the idea!

I'll get to making that change in the coming days

@jbms
Copy link
Collaborator

jbms commented Nov 6, 2023

Please resolve merge conflicts and then should be ready.

@seankmartin
Copy link
Contributor Author

@jbms merge conflicts fixed!

@jbms jbms merged commit 72d5c0c into google:master Nov 6, 2023
16 of 20 checks passed
@seankmartin seankmartin deleted the feat/variable-ray-samples branch January 25, 2024 16:52
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.

None yet

2 participants