Skip to content
This repository has been archived by the owner on Sep 14, 2021. It is now read-only.

Numerous updates #8

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Numerous updates #8

wants to merge 26 commits into from

Conversation

lukehutch
Copy link

Viewport now needs an id

Copy link
Owner

@m0glan m0glan left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. pcl-java v0.5.0 relies upon PCL 1.8.1; a new version would have to be worked on for 1.9.1 support.

@lukehutch
Copy link
Author

Right -- although this is the only API breakage I ran into (it builds fine with this one change). I didn't make a corresponding update to the README.md file to explain that PCL 1.9.1 was needed.

Separate question -- by chance do you have any support for the PCL pointcloud registration methods already written but not checked in? That's what I originally needed, but I didn't discover until I built the code that not much of PCL is covered yet.

@m0glan
Copy link
Owner

m0glan commented Aug 14, 2019

@lukehutch I don't. You are right when you say not much of PCL is covered. I started this project for my former employer with a limited predefined list of features. It's a shame I don't have time to work on this project anymore nowadays. I'll think of a solution or try to delegate the administration of this project to someone else.

@lukehutch
Copy link
Author

lukehutch commented Aug 14, 2019

Thanks, it is a shame -- I was excited to find the project, and assumed more was covered based on the README. I tried some other solutions too to generate a Java API, including Panama (which doesn't support C++ yet) and JNAerator (which didn't support some of the features of the PCL header files). I suppose I'll have to bite the bullet and get my hands dirty with C++!

@m0glan
Copy link
Owner

m0glan commented Aug 14, 2019

Good luck. I'm hoping to return to this project one day since I like the idea a lot. Code is much cleaner and readable in Java than C++ which is usually very attractive for academics.

@lukehutch
Copy link
Author

Yes, and it is much, much easier to combine together code from different libraries in Java, by just pulling in a Maven dependency. I think you're right that this would help a lot of academics, but also hobbyists. It's unfortunate that a lot of the electronics and robotics hobby platforms require hobbyists to learn C/C++, since they require a lot of arcane knowledge to use properly.

@m0glan
Copy link
Owner

m0glan commented Aug 14, 2019

Perhaps a shame, but when it comes to costly operations such as pc calculations on millions of points, c++ had an undeniable advantage in performance.

@lukehutch
Copy link
Author

BTW the Panama team have started to work on creating Java bindings for C++ libraries automatically, leveraging Clang to compile C++ headers into Java classes. (This works really well for C already.) If you're interested in contributing to that instead, it would solve the problem not just for PCL, but other libraries too. I might try to contribute to this too when I get some more time.

@m0glan
Copy link
Owner

m0glan commented Aug 14, 2019

Sounds great, thanks for that

@lukehutch
Copy link
Author

lukehutch commented Aug 21, 2019

Oops, I'm not sure why pushing to my master branch now sends all commits to this PR! I suppose I needed to create one branch per change? Sorry that these changes are not related to the original PR. I'm not sure how to split them out...

Most of the changes above were to make alloc() called automatically by the NativeObject constructor (following RAII), and to make NativeObject implement AutoCloseable (following Java conventions for resources that need freeing).

@lukehutch lukehutch changed the title Update for PCL 1.9.1 API Numerous updates Aug 22, 2019
@lukehutch
Copy link
Author

I haven't figured out yet why all my pushes are ending up in this PR, so I just changed the title of the PR :-D

There was a bit of back and forth during all the commits, so it might be best just to look at a diff of your tree against the final tree in the PR, to see what changes you want to merge, if any.

My goal was to enable point cloud registration through pcl-java, but actually I may not have time to test that now, as I am realizing that the overhead required to support the PCL features I need through JNI requires more overhead than I can really afford, so I'm looking at some other ways to get the pointcloud registration done. (In other words, I may not be able to commit more time to this, but I gave it the college try!)

@m0glan
Copy link
Owner

m0glan commented Aug 22, 2019

Hi, sorry for my slow replies, work. The git workflow you should follow is feature branching. Essentially:

  1. Make sure your master is up-to-date with origin (git checkout master && git pull);
  2. Create a branch for your feature or bug-fix:
git checkout -b feature/normal-estimation

or

git checkout -b bugfix/memory-issue-fix
  1. Push branch to the origin of your fork:
git push --set-upstream origin <branch>
  1. Make your feature related changes on the feature branch, add, commit and push them to the origin.

  2. Create a pull request from your feature branch (origin of your fork) to the master of the original project.

That's it. This will make changes much easier for me to review over the next weekend since I can take a look at them individually.

@m0glan
Copy link
Owner

m0glan commented Aug 22, 2019

Also, I've skimmed through your additions, looks good ;)

@lukehutch
Copy link
Author

Thanks, that's very helpful for future reference, and I appreciate the doc link!

Unfortunately I won't have the bandwidth to go back and split these changes apart for at least a couple of weeks, as I have to devote my time to another project. Sorry for the mess!

@oberdanpinheiro
Copy link

@vmoglan do I need to use the IterativeClosestPoint class do you have pcl_java.dll PCL 1.9.1?
Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants