-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
freecad: add support for PCL #398393
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
base: master
Are you sure you want to change the base?
freecad: add support for PCL #398393
Conversation
PCL supports generating surfaces from point clouds. FreeCAD can be built with PCL support. Generating surfaces within FreeCAD is possible through the 'Reverse Engineering' Toolbox. This support could be added without bumping/changing the FreeCAD version. But, that would involve packaging an older version of PCL (1.14) which is undesirable. I tested this by generating a simple point cloud (.asc format) ```asc $ cat ~/freecad/projects/foo/surface.asc 0 0 0 1 1 1 -1 -1 1 1 -1 1 -1 1 1 ``` and using the Reverse Engineering → Surface Reconstruction → Poisson... flow and saw my point cloud convert into a surface. Without these changes, the menu item "Poison..." was greyed out. So, it seems to be working as intended since: 1. It's no longer greyed out in the UI. 2. I'm able to convert the above point cloud into a surface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff but I'm not sure it should be merged as-is, I think it would be preferable to keep 1.0.0 available. Ideally this PR would be exposed as another package such as freecad-with-PCL
(or something along those lines) with a package override option to build withPCL
support being exposed on the main package.
@srounce Sure, yeah. I'm not trying to bump the version or get this merged, now. It's in draft. Whenever FreeCAD bumps a new release we can refactor a bit and add a new package. But, why not add libpcl by default? I didn't compare binary sizes but I wouldn't expect it to be a big cost by any measurement. |
Sounds good! As for PCL there's no reason why not, it was just overlooked. If you're up for moving your PCL addition changes to another PR it'd be much appreciated. |
@srounce sorry, I'm a bit confused. What do you want the new PR to do? This PR adds libpcl support to FreeCAD. Earlier you were suggesting to break out a package variant. But, if the build time or binary size cost isn't high by adding it then I'd prefer to add it by default, like in this PR. Maybe this other/new PR you're talking about would look to add support to FreeCAD 1.0.0. But, I mentioned above that this is annoying since the required version of PCL was never packaged for NixOS and I want to avoid a potential rabbit hole. |
Note: This is in draft until a new FreeCAD release/tag is made available (last one was ~5 months ago). But, I'm posting this draft work in case anyone wants to do similar stuff with PCL 1.14 or take responsibility for this work once a new FreeCAD release/tag is cut.
PCL supports generating surfaces from point clouds. FreeCAD can be built with PCL support. Generating surfaces within FreeCAD is possible through the 'Reverse Engineering' Toolbox. This support could be added without bumping/changing the FreeCAD version. But, that would involve packaging an older version of PCL (1.14) which is undesirable.
I tested this by generating a simple point cloud (.asc format)
and using the Reverse Engineering → Surface Reconstruction → Poisson... flow and saw my point cloud convert into a surface. Without these changes, the menu item "Poison..." was greyed out. So, it seems to be working as intended since:
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.
Ping @srounce (maintainer)
Ping @ein-shved (I removed his patch file because a lot of changes looked already-integrated upstream)