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

Added isocontour shader #40

Merged
merged 2 commits into from
Nov 8, 2021
Merged

Added isocontour shader #40

merged 2 commits into from
Nov 8, 2021

Conversation

sochowski
Copy link
Contributor

  • Created isocontour.frag.glsl that renders points within the range of iso_tolerance from any iso_layers[i], where i is in the range of [0, iso_num_layers)
  • Added slider to adjust iso_tolerance
  • Added buttons to add and remove isocontour layers, as well as the ability to input up to 32 distinct layers

Comment on lines 90 to 95
for i in range(len(self.iso_layers)):
changed, self.iso_layers[i] = imgui.input_float(
"Layer " + str(i + 1),
self.iso_layers[i],
flags=imgui.INPUT_TEXT_ENTER_RETURNS_TRUE,
)
Copy link
Member

Choose a reason for hiding this comment

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

We want to keep the idiom here of always updating changed:

Suggested change
for i in range(len(self.iso_layers)):
changed, self.iso_layers[i] = imgui.input_float(
"Layer " + str(i + 1),
self.iso_layers[i],
flags=imgui.INPUT_TEXT_ENTER_RETURNS_TRUE,
)
for i in range(len(self.iso_layers)):
_, self.iso_layers[i] = imgui.input_float(
"Layer " + str(i + 1),
self.iso_layers[i],
flags=imgui.INPUT_TEXT_ENTER_RETURNS_TRUE,
)
changed = changed or _

Comment on lines 242 to 245
p._set_uniform(
"iso_layers",
np.asarray(self.iso_layers + [0.0] * (32 - len(self.iso_layers))),
)
Copy link
Member

Choose a reason for hiding this comment

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

What if we instead had an array that we filled, like:

Suggested change
p._set_uniform(
"iso_layers",
np.asarray(self.iso_layers + [0.0] * (32 - len(self.iso_layers))),
)
v = np.zeros(32, dtype="float32")
v[:len(self.iso_layers)] = self.iso_layers
p._set_uniform(
"iso_layers",
v
)

Copy link
Member

Choose a reason for hiding this comment

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

(this will need reformatting)

@@ -0,0 +1,146 @@
in vec4 v_model;
Copy link
Member

Choose a reason for hiding this comment

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

We chatted briefly about this on the call, but this may be workable as a ray_tracing-type fragment shader. Not necessary to change it now, though.

Copy link
Contributor

@chrishavlin chrishavlin left a comment

Choose a reason for hiding this comment

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

Great work! Left one idea on arranging the "Remove Layer" button in line for each layer.

And I thought some more about how to set isocontour values in raw data values. So at least in the case of the block rendering, the data is normalized when building the data Texture objects (in scene_data.block_collection.BlockCollection._load_textures), so when you send iso_layers to the shader it will expect values in the range 0,1. So what I think we should do is add a _get_sanitized_iso_layers method to SceneComponent. The method of normalizing data might change depending on the scene_data type, so we should just pass through the values in base_component.SceneComponent:

def _get_sanitized_iso_layers(self):
     return self.iso_layers

But then override in child classes. So for scene_components.blocks.BlockRendering we'd want to add something like:

def  _get_sanitized_iso_layers(self):
     # return the sanitized layers

     # pull extrema from `BlockCollection`, these are extrema across all blocks
     data_min = self.data.min_val  
     data_max = self.data.max_val  
     
     # apply the same scaling as in `self.data._load_textures()`
     normalized_isovals = [ (iso_val - data_min) / (data_max - data_min) for iso_val in self.iso_layers]
     return normalized_isovals

And then we'd want to send those normalized iso_layers to the shader program instead of the raw iso_layers values from the user input.

yt_idv/scene_components/base_component.py Outdated Show resolved Hide resolved
changed, self.iso_layers[i] = imgui.input_float(
imgui.columns(2, "iso_layers_cols", False)
i = 0
while i < len(self.iso_layers):
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to figure out how to manage this without a while loop, but I can't quite come up with something. So I suppose, this is OK! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think this is good for now! But I also agree that there could a way to simplify this... maybe separating the initial table construction from row removal when a button is pushed? Would result in more code but might be conceptually simpler. But ya, I say leave this as is for now.

Copy link
Contributor

@chrishavlin chrishavlin left a comment

Choose a reason for hiding this comment

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

This version looks good! One small code suggestion to add a comment.

@@ -260,3 +295,6 @@ def run_program(self, scene):

def draw(self, scene, program):
raise NotImplementedError

def _get_sanitized_iso_layers(self):
Copy link
Contributor

@chrishavlin chrishavlin Nov 3, 2021

Choose a reason for hiding this comment

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

Suggested change
def _get_sanitized_iso_layers(self):
def _get_sanitized_iso_layers(self):
# child scene_component classes override this to normalize layer values if needed

changed, self.iso_layers[i] = imgui.input_float(
imgui.columns(2, "iso_layers_cols", False)
i = 0
while i < len(self.iso_layers):
Copy link
Contributor

Choose a reason for hiding this comment

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

I also think this is good for now! But I also agree that there could a way to simplify this... maybe separating the initial table construction from row removal when a button is pushed? Would result in more code but might be conceptually simpler. But ya, I say leave this as is for now.

@chrishavlin chrishavlin merged commit 27f0607 into yt-project:main Nov 8, 2021
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