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

opening .obj #8

Closed
FedeClaudi opened this issue May 26, 2020 · 10 comments
Closed

opening .obj #8

FedeClaudi opened this issue May 26, 2020 · 10 comments

Comments

@FedeClaudi
Copy link
Contributor

Hey everyone,

Currently .obj files with mesh data are opened and returns as triplets of vertices, faces and edges, which is not very useful.

Given that meshes are used almost exclusively for visualisation (in brainrender) I'd suggest opening them with vtkplotter.vtkio.load, which returns an instance of Mesh from vtkplotter which has loads of useful function. A lot of functionality currently in brainrender which will be moved to brainatlas-api relies on the Mesh class.

An alternative would be to use meshpart, but to be honest I haven't used it too much, don't know how good it is.

The only downside I can see is that this will require adding vtkplotter to the requirements which comes with a bunch of dependencies of it's own. It doesn't cause problems in my experience but it does make the package less lightweight.

What do people think?

@vigji
Copy link
Member

vigji commented May 26, 2020

I would definitively not add vtkplotter as a requirement in this package!

Actually, I think that the current representation of the mesh data is the one that makes the most sense to expose in this package, as everything more specific should be specified in visualisation tools that can load .obj files as vtkplotter does. Plus, many applications for mesh visualisation support a straightforward mesh instantiation from the triplet of vertices, faces and edges. I have not tried it, but from the documentation of vtkplotter it seems that the Actors can also be instantiated from a list of [vertices, faces], although I have not tested it!

@FedeClaudi
Copy link
Contributor Author

Yeah you can do that, it just seems like a bit of a roundabout way of going about this.
You can get the position of vertices and faces out of a vtkactor as well if needed, but I think that hardly ever would one need to work directly with that.

Also a lot of functions like get_region_unilateral, get_region_center_of_mass etc work nicely with vtkplotter and I wouldn't be too keen to re-write them from scratch

@FedeClaudi
Copy link
Contributor Author

either way it's not a huge problem given that we have methods like get_mesh_file_from_name as well

@vigji
Copy link
Member

vigji commented May 26, 2020

uhm, I see your point!
I will see whether there is a lighter dependency that can solve the problem as neatly as vtkplotter would. In the meantime, maybe we could have vtkplotter as an optional dependency so that all parts that don't need mesh operations could work without it?

@vigji
Copy link
Member

vigji commented May 26, 2020

probably trimesh would be a reasonable dependency to handle these mesh operations. What are the potential operations that you have encountered working with region meshes so far?
I can see already:

  1. Make a mesh unilateral;
  2. check if point is contained in a region (could also be handled with the raster region annotation stack probably);

@FedeClaudi
Copy link
Contributor Author

of the top of my head:

  1. get random points in mesh (useful for debugging sometimes)
  2. get mesh center of mass

2, 3, 4 are implemented by default in vtkplotter Mesh ;)

@FedeClaudi
Copy link
Contributor Author

also this might be useful: https://github.com/nschloe/meshio

@vigji
Copy link
Member

vigji commented May 26, 2020

So, maybe the most convenient path is the following: we temporarily add the optional vtkplotter dependency, have tests working for the operations that require it, and then try to purge it out without affecting the test results.

@adamltyson
Copy link
Member

Sounds good. I'd like to avoid large dependencies as far as possible (in the end we may have 10+ packages that need to be interoperable), but there's no reason not to have them now and try to remove later.

@FedeClaudi
Copy link
Contributor Author

sounds like we found an agreement, will close for now

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

No branches or pull requests

3 participants