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

WIP: Isolayer updates #140

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

chrishavlin
Copy link
Contributor

Some updates to the isolayer functionality:

  • moves all the isolayer logic to its own class to be used as a mixin
  • isolayer tolerances can now be set by layers
  • layer min/max values (layer value +/-tolerance/2) are now pre-calculated, avoiding some potentially confusing logic that was causing some issues...
  • added example
  • updated test

Close #138

@chrishavlin chrishavlin marked this pull request as draft July 1, 2024 21:58
from yt_idv.scene_data.block_collection import BlockCollection
from yt_idv.shader_objects import component_shaders


class BlockRendering(SceneComponent):
class BlockRendering(SceneComponent, Isolayers):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthewturk any opinio on using a multi-inheritence structure here? was the most convenient way i saw to add the isolayer functionalty to the block rendering while allowing future re-use with octree blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(which is why we had initially added it to the base component I think)

Copy link
Member

Choose a reason for hiding this comment

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

hmm, good point. I guess I'm OK with it, but I also wonder if this should just be either a different shader (which I guess would open up multiple issues with having new uniforms that are conditionally modified) or a different component referencing the same data object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a different component is actually a great idea. And we could have a "add isolayer" button similar to the button for adding a grid outline. And it would allow viewing both isolayers and a volume rendering at once (though I'm not sure this would work right away... Offhand I don't remember how the different components are blended, might be some depth buffer issues...).

@chrishavlin
Copy link
Contributor Author

@pre-commit.ci autofix

Comment on lines +21 to +26
box_width = traitlets.CFloat(0.1)
sample_factor = traitlets.CFloat(1.0)
transfer_function = traitlets.Instance(TransferFunctionTexture)
tf_min = traitlets.CFloat(0.0)
tf_max = traitlets.CFloat(1.0)
tf_log = traitlets.Bool(True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not positive that all these are needed -- copied over from the block component, need to check whether they're needed by the shader.

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.

Isolayer stuff should be in its own component
2 participants