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

Bug: move does not support multi-volume bodies. #74

Open
lukethehuman opened this issue Sep 24, 2024 · 2 comments
Open

Bug: move does not support multi-volume bodies. #74

lukethehuman opened this issue Sep 24, 2024 · 2 comments
Labels
bug Something isn't working
Milestone

Comments

@lukethehuman
Copy link
Collaborator

Unexpected error encountered when supplying a non-zero origin argument to an assembly class (i.e. subclass of CreatedComponentAssembly), which makes use of a component class (i.e. SimpleComponent subclass) whose make_geometry method returns a CubitInstance object representing a multi-volume body.

Expected behaviour: the whole assembly should be translated to the co-ordinates provided, including any multi-volume components.

Actual behaviour: a error is raised by cubit as below. (These volumes are not translated but others in the assembly are moved as expected. An error is not raised by blobmaker, though check_for_overlaps will raise an error in the case that not moving the volumes in question leads to overlaps.)

ERROR: Volume 5 is part of a multi-volume body.
       Cannot move Volume 5 by itself
ERROR: Volume 6 is part of a multi-volume body.
       Cannot move Volume 6 by itself

The error is likely triggered by the move function, which attempts to move one of the volumes by itself rather than the whole body. We may wish to modify this function to detect when it is dealing with a multi-volume body and move it accordingly.

In the case where I encountered this bug, upon discussing the problem, it made more sense to bypass the issue by removing the union operation which created the multi-volume body and instead return both volumes separately in a list i.e. return [volume_1, volume_2].

If we wish to keep the existing functionality where multi-volume bodies are not supported, we should add something to the documentation to guide users towards returning volumes separately for component subclasses containing more than one volume.

@lukethehuman lukethehuman added the bug Something isn't working label Sep 24, 2024
@sid-mungale sid-mungale added this to the open source milestone Oct 28, 2024
@sid-mungale
Copy link
Collaborator

cubit doesn't allow movement of volumes independently if they are part of a multi-volume body (MVB). i also don't want to force-move the entire MVB if someone attempts to move one of it's children bodies. i have thought of the following options:

  • provide a 'body' argument to move to first convert geometries to bodies. this has the side-effect of possibly adding new volumes to a component/ geometry if a volume is part of an MVB, which doesnt sit right with me.
  • ask users to avoid MVBs as a whole, maybe even adding checks to union for this. this may be too restrictive.
  • ask users to handle MVBs as bodies instead of volumes, i.e. operate on body 1 instead of volume 1 and volume 2. this would require users to know when an MVB is created.

@lukethehuman
Copy link
Collaborator Author

I think if we don't envision any cases where we need a multibody volume, keeping hypnos' API simpler by avoiding them is valid. The only issue is that it's currently fairly easy to accidentality create them using union if you're a user that's not especially familiar with cubit (like me). If there is a way to allow handling MVBs as bodies instead of volumes without significantly expanding the API that could be better, but otherwise we can apply YAGNI (you ain't gonna need it) and go for the simplest option here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants