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

Fix indices_to_points to make it consistent to VoxelMesh.as_boxes #260

Merged
merged 6 commits into from Nov 25, 2018

Conversation

wkentaro
Copy link
Contributor

I'm not so sure, but usually voxel point coordinate should be calculated as:

(index - 0.5) * pitch + origin = index * pitch + origin - pitch / 2.0

In VoxelMesh.as_boxes this formulation is used, but the matrix_to_points function does not.

@wkentaro wkentaro changed the title Fix indices_to_points consistent in VoxelMesh.as_boxes Fix indices_to_points to make it consistent in VoxelMesh.as_boxes Nov 24, 2018
@wkentaro wkentaro changed the title Fix indices_to_points to make it consistent in VoxelMesh.as_boxes Fix indices_to_points to make it consistent to VoxelMesh.as_boxes Nov 24, 2018
@mikedh
Copy link
Owner

mikedh commented Nov 25, 2018

Yeah, you're right it should definitely be consistent. Maybe we should make two functions, points_to_indices and indices_to_points to avoid screwing up the logic, then we can add a round trip or two into unit tests.

Some quick thoughts:

  • probably want to make sure indices is float before doing the operation, and check the other inputs
  • in as_boxes, it needs to be self.pitch, not pitch
  • generally probably want to use kwargs rather than args when calling functions (pitch=self.pitch vs pitch), makes it more explicit and CI will start failing if someone decides to rearrange args.

trimesh/voxel.py Outdated Show resolved Hide resolved
trimesh/voxel.py Outdated Show resolved Hide resolved
@mikedh mikedh changed the base branch from master to voxel_merge November 25, 2018 20:10
@wkentaro
Copy link
Contributor Author

I added points_to_indices and tests for both functions. Could you review again?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 84.096% when pulling a83f42f on wkentaro:indices_to_points into 5953bed on mikedh:voxel_merge.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 84.096% when pulling a83f42f on wkentaro:indices_to_points into 5953bed on mikedh:voxel_merge.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 84.096% when pulling a83f42f on wkentaro:indices_to_points into 5953bed on mikedh:voxel_merge.

@coveralls
Copy link

coveralls commented Nov 25, 2018

Coverage Status

Coverage decreased (-0.003%) to 83.974% when pulling a83f42f on wkentaro:indices_to_points into 5953bed on mikedh:voxel_merge.

@mikedh
Copy link
Owner

mikedh commented Nov 25, 2018

Looks great, thanks!

@mikedh mikedh merged commit 60425c8 into mikedh:voxel_merge Nov 25, 2018
@wkentaro wkentaro deleted the indices_to_points branch November 25, 2018 22:30
LinJiarui added a commit to smartaec/trimesh that referenced this pull request Apr 4, 2019
mikedh added a commit that referenced this pull request Apr 4, 2019
fix make sparse_solid consistent to changes of indices_to_points made by pull request #260
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.

None yet

3 participants