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

Feature/additional geometry file export methods #225

Merged
merged 64 commits into from
Sep 13, 2024

Conversation

josephburkhart
Copy link
Contributor

@josephburkhart josephburkhart commented Jun 20, 2024

Description

I have added a some methods to the shapes.Polyhedron class to allow users to export 3D geometry to a handful of common file formats. None of these methods introduce additional dependencies. The following file formats would be supported:

  • .obj Wavefront OBJ
  • .off Object File Format
  • .stl Stereolithography
  • .ply Polygon File Format
  • .x3d Extensible 3D
  • .vtk Legacy VTK
  • .html HTML (embedded X3D in a simple HTML document)

Note: this is a DRAFT PULL REQUEST. With input from other collaborators, I may still make significant changes to the new methods.

Motivation and Context

This change could make it significantly easier for users to visualize their geometries and export them for rendering and other publication-related tasks. Currently, users have to hack together their own export functions if they want to export their geometries to, say, blender.

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

@josephburkhart josephburkhart added the enhancement New feature or request label Jun 20, 2024
@josephburkhart
Copy link
Contributor Author

Planning to start drafting some tests tomorrow.

@josephburkhart
Copy link
Contributor Author

In a conversation offline, @janbridley suggested putting this functionality in a separate file, rather than attaching it via methods to the shapes.Polyhedron class.

Currently, the implementation only works for polyhedrons, because it requires explicitly defined vertices and faces. If we put export functions into a separate file, it may be reasonable to allow them to accept any subclass of shapes.Shape3D. If we wanted to do that, we would need a way to generate vertices and faces to approximate the surfaces of shapes.Ellipsoid and shapes.Sphere. If any folks have suggestions on previous implementations of such calculations in coxeter's current dependencies, I'd appreciate it. I could implement such calculations myself, but I expect someone else has already done this.

@janbridley
Copy link
Collaborator

In a conversation offline, @janbridley suggested putting this functionality in a separate file, rather than attaching it via methods to the shapes.Polyhedron class.

Currently, the implementation only works for polyhedrons, because it requires explicitly defined vertices and faces. If we put export functions into a separate file, it may be reasonable to allow them to accept any subclass of shapes.Shape3D. If we wanted to do that, we would need a way to generate vertices and faces to approximate the surfaces of shapes.Ellipsoid and shapes.Sphere. If any folks have suggestions on previous implementations of such calculations in coxeter's current dependencies, I'd appreciate it. I could implement such calculations myself, but I expect someone else has already done this.

As a simpler alternative, we could have the export code in a separate file that is called as aPolyhedron method. This gets us the development and maintenance benefits without requirements additional approximations for rounded shapes. If those approximations are something we wanted, then this approach seems perfect. I do know that mesh approximations of concave curved shapes were brought up offline recently, although generalizations of this sort are nontrivial.

@josephburkhart
Copy link
Contributor Author

pre-commit.ci autofix

for f in shape.faces:
# Decompose face into triangles
# ref: https://stackoverflow.com/a/66586936/15426433
triangles = [[vs[f[0]], vs[b], vs[c]] for b, c in zip(f[1:], f[2:])]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a nice little snippet

coxeter/io.py Outdated Show resolved Hide resolved
coxeter/io.py Outdated
coordinate_indices.insert(len(f) + prev_index, -1)
prev_index += len(f) + 1

coordinate_points = [v for f in shape.faces for i in f for v in shape.vertices[i]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider rewriting this with numpy functions - ravel, etc. Should be possible to simplify!

tests/test_io.py Outdated Show resolved Hide resolved
@janbridley janbridley force-pushed the Feature/Additional-geometry-file-export-methods branch from 5415ab4 to fa10da7 Compare September 12, 2024 18:26
@josephburkhart josephburkhart force-pushed the Feature/Additional-geometry-file-export-methods branch from fa10da7 to 6e28a64 Compare September 13, 2024 13:30
@josephburkhart josephburkhart force-pushed the Feature/Additional-geometry-file-export-methods branch from 121f496 to f260fd2 Compare September 13, 2024 13:41
@josephburkhart josephburkhart force-pushed the Feature/Additional-geometry-file-export-methods branch from af49d65 to d8b7726 Compare September 13, 2024 13:53
@josephburkhart
Copy link
Contributor Author

@janbridley all the tests are now passing, thanks to @joaander 's help. As the code currently stands, we would need to re-generate the control files with every version release, since the export functions include the coxeter version number in some of the files. There's an if __name__ == "__main__" block in tests/test_io.py that generates new control files, so a call to this file would need to be added to the version release workflow. If that sounds like too much of a hassle, I could add in some functionality that ignores coxeter version numbers during the file comparison check, but this might complicate adding support for new file formats in the future.

@josephburkhart josephburkhart marked this pull request as ready for review September 13, 2024 14:09
Copy link
Collaborator

@janbridley janbridley left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for this

@janbridley
Copy link
Collaborator

@janbridley all the tests are now passing, thanks to @joaander's help. As the code currently stands, we would need to re-generate the control files with every version release, since the export functions include the coxeter version number in some of the files. There's an if __name__ == "__main__" block in tests/test_io.py that generates new control files, so a call to this file would need to be added to the version release workflow. If that sounds like too much of a hassle, I could add in some functionality that ignores coxeter version numbers during the file comparison check, but this might complicate adding support for new file formats in the future.

It should be fairly simple to automate running that file with the CI. I'll add this in the next release PR. I think the comparison function is good for now, and we can always replace the contents of that function in the future for more detailed/version-agnostic feedback.

@janbridley janbridley enabled auto-merge (squash) September 13, 2024 15:02
@janbridley janbridley merged commit ceeb6e3 into master Sep 13, 2024
9 checks passed
@janbridley janbridley deleted the Feature/Additional-geometry-file-export-methods branch September 13, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants