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

What happened to intersection_sphere_sphere #1381

Closed
yck011522 opened this issue Jul 11, 2024 · 3 comments
Closed

What happened to intersection_sphere_sphere #1381

yck011522 opened this issue Jul 11, 2024 · 3 comments

Comments

@yck011522
Copy link
Contributor

@tomvanmele

I noticed in #1340 , you have changed two lines in the function of intersection_sphere_sphere(sphere1, sphere2) to go back to an earlier implementation.
Specifically here:
https://github.com/compas-dev/compas/pull/1340/files#diff-521558cf7170fc7e330b54e5f23c6c50b25dbf147efd2705b7c21e960888aca2L496-L497

    center1, radius1 = sphere1.base, sphere1.radius
    center2, radius2 = sphere2.base, sphere2.radius

back to this:

    center1, radius1 = sphere1
    center2, radius2 = sphere2

This was actually a problem fixed in an earlier issue #1281 where the function would not work with a proper Sphere object. The Sphere object cannot be iterated to give center and radius. This function is breaking something in compas_fab. Is there a reason to go back to that earlier unpack?

@tomvanmele
Copy link
Member

yes, the lower level functions are meant to work with pure Python inputs and not rely on the structure of the higher level classes.

the above fix was not consistent with this and therefore created unexpected behaviour.

@yck011522
Copy link
Contributor Author

Then does that mean that the Sphere class should be changed to allow center1, radius1 = sphere1 to work?

@tomvanmele
Copy link
Member

no, there is no meaningful way in which this can be done. if the sphere/sphere intersection doesn't exist yet on the sphere class it should be added. ideally your codebase then uses one of the two APIs exclusively...

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

2 participants