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

boxClipper3D tutorial #5762

Closed
wants to merge 2 commits into from
Closed

boxClipper3D tutorial #5762

wants to merge 2 commits into from

Conversation

patcmorneau
Copy link

I could not figure out a way to see if my .rst file is ok.

http://sphinx.pocoo.org/rest.html is down

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Hi, to do what you describe in this tutorial, I would recommend CropBox instead. Personally, I haven't used BoxClipper3D much, but it seems to me that CropBox is used more often, better tested, and is clearer regarding whether transformations are applied to the box or the cloud

};


int main(int argc,char** argv){
Copy link
Member

Choose a reason for hiding this comment

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

Note: argc and argv are unused, that is why the documentation check fails

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, i will look at the CropBox, it might suit more want we want to do.

@mvieth
Copy link
Member

mvieth commented Jul 22, 2023

@patcmorneau Hi again, did you have a chance to test CropBox? If you agree that CropBox is to be preferred over BoxClipper3D for this use case, I would rather not add a tutorial that recommends the use of BoxClipper3D.

Additionally, I opened a pull request to change the behaviour of the BoxClipper3D constructor you use in this tutorial. That pull request addresses the "counter intuitive-ness" you also mentioned, which was also reported in an issue a few years ago. For further reasoning, please see my pull request. If my pull request is merged, this tutorial would be out of date.

So to conclude, I am not sure if it is a good idea to add this tutorial. What do you think?

@patcmorneau
Copy link
Author

Yes the cropBox is what we were looking for, thank you !
Also I don't know if its worth mentioning in the documentation that the rotation are in radians.

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.

2 participants