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 option to serialize all vtkDataset arrays #1596

Merged
merged 5 commits into from
Sep 28, 2020
Merged

Conversation

xavArtley
Copy link
Collaborator

@xavArtley xavArtley commented Sep 21, 2020

A vtk dataset can contain several data arrays but only one is used to color the mesh, the "active scalars".
Until now only this array was serialized, it allow to minimize the size of the serialized scene.
However by allowing the serialization of all data arrays and using some custom javascript functions we could make some temporal 3d visualization easier.
This PR add a parameter serialize_all_data_arrays to allow this
Here is a comparison between both options:
vtk_all_arrays

@codecov
Copy link

codecov bot commented Sep 21, 2020

Codecov Report

Merging #1596 into master will decrease coverage by 0.34%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1596      +/-   ##
==========================================
- Coverage   85.68%   85.34%   -0.35%     
==========================================
  Files         147      147              
  Lines       16460    16705     +245     
==========================================
+ Hits        14104    14257     +153     
- Misses       2356     2448      +92     
Impacted Files Coverage Δ
panel/pane/vtk/synchronizable_serializer.py 80.84% <ø> (-3.87%) ⬇️
panel/pane/vtk/vtk.py 94.19% <ø> (+0.01%) ⬆️
setup.py 0.00% <0.00%> (ø)
panel/auth.py 38.99% <0.00%> (+4.75%) ⬆️
panel/io/server.py 74.57% <0.00%> (+8.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64a0755...d29c50b. Read the comment docs.

# if backPropertyInstance:
# instance['dependencies'].append(backPropertyInstance)
# instance['calls'].append(['setBackfaceProperty', [wrapId(backfacePropId)]])

Copy link
Member

Choose a reason for hiding this comment

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

Keep this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes we can keep this if vtkjs is going to support backface color we won't have to rethink about it

panel/pane/vtk/vtk.py Outdated Show resolved Hide resolved
@philippjfr
Copy link
Member

Apart from a few comments this looks fine to me. However I do worry about continuing to expand the custom serializer so just checking where are we on using the native vtk serializer?

Co-authored-by: Philipp Rudiger <prudiger@anaconda.com>
@xavArtley
Copy link
Collaborator Author

xavArtley commented Sep 28, 2020

Apart from a few comments this looks fine to me. However I do worry about continuing to expand the custom serializer so just checking where are we on using the native vtk serializer?

using the function VTK.import_scene , we could use any serializer which create a file wich can be read in https://kitware.github.io/vtk-js/examples/SynchronizableRenderWindow.html
actually not since we reuse panel serializer in all cases

@philippjfr
Copy link
Member

Okay, merging for now.

@philippjfr philippjfr merged commit b151ae6 into master Sep 28, 2020
@philippjfr philippjfr deleted the xav/vtk_arrays branch September 28, 2020 14:17
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.

2 participants