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

correct dockerfile & upgrade to nvidia-docker2 #40

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

Conversation

ianchen-tw
Copy link
Contributor

Conclusion

  1. support nvidia-docker2
  2. a new dockerfile that can finally be built
  3. can run on ubuntu 18.04 now
  4. Add documentation for RealSense2 camera
  5. Add documentation for camera calibration
  6. fix some bugs

Bug fix

  1. the cdlf command can work now
  2. Support variable length of object-models

@peteflorence
Copy link
Collaborator

peteflorence commented Nov 27, 2018 via email

Copy link
Member

@patmarion patmarion left a comment

Choose a reason for hiding this comment

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

Ian, thank you! This is great. Please give us some time to review this. I am happy to upgrade labelfusion to require nvidia-docker2, hopefully most people are ok with this upgrade.

In some of my in-line comments, I asked if you could move some of the bug fixes into their own pull requests. If you don't have time for that, let me know, I can grab your branch and make the PRs myself (when I have some more time :) )

#

#image_name=robotlocomotion/labelfusion:latest
image_name=ianre657/labelfusion:16.04-latetest
Copy link
Member

Choose a reason for hiding this comment

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

For this PR, I would like to change the image name back to the official name. I can build the branch and push the new image before landing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good to me, just change it as you wish.

@@ -1,11 +1,108 @@
FROM nvidia/cuda:8.0-devel-ubuntu16.04
FROM ianre657/cuda8gl:latetest
Copy link
Member

Choose a reason for hiding this comment

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

what is in your image? we'd probably set this to nvidia_cuda_9.0-devel-ubuntu16.04 but could consider ubuntu 18. Is there a reason you depend on the ubuntu version of the labelfusion image?

Copy link
Contributor Author

@ianchen-tw ianchen-tw Nov 28, 2018

Choose a reason for hiding this comment

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

There exist an issue that nvidia haven't support OpenGL in it's official docker image.
NVIDIA/nvidia-docker#534
and they create another docker image to address this issue, it's called cudagl.
https://hub.docker.com/r/nvidia/cudagl/
but, you can find out that the version of cuda in cudagl is 9.2, which the most GPUs driver on the market haven't supported for that (9.1 seems to be the mainstream), and if you look inside the dockerfile of nvidia's cudagl(with CUDA9.2), there's also an comment as below.
# CUDA 9.2 is not officially supported on ubuntu 18.04 yet, we use the ubuntu 17.10 repository for CUDA instead.
So I decide to create my own cudagl(CUDA8.0), also.... Can’t come up with the reason why I choose CUDA8.0 rather than9.1.
It's here, I just copy most of the code from cuda8.0 dockerfile and follow the instrucitons from the first link.
https://github.com/ianre657/cuda8gl


# compile director
#RUN /tmp/build_scripts/compile_director.sh
RUN git clone -b labelFusion-director https://github.com/ianre657/director.git \
Copy link
Member

Choose a reason for hiding this comment

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

I have recently added ubuntu18 support to Director, but not landed it in master. Hopefully I can get things up-to-date in Director's master branch, then this PR could be updated to reference that. Are there other changes you put into your fork here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's about the dependency of director, apriltags_driver

ianchen-tw/director@a98c56e
the author have update a new version of apriltags_driver, which fix some bugs

#RUN /tmp/build_scripts/compile_elasticfusion.sh
ARG root_dir=/root
ARG install_dir=/root/install
RUN git clone -b pf-lm-debug-jpeg https://github.com/ianre657/ElasticFusion.git \
Copy link
Member

Choose a reason for hiding this comment

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

I see you changed ElasticFusion to your fork, too. What are you fixing in your fork, and what changes are required? Could this fixes to upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original ElasticFusion use gcc5 to compile the project, it's kind out outdated for 18.04.

Copy link
Contributor Author

@ianchen-tw ianchen-tw Nov 28, 2018

Choose a reason for hiding this comment

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

also, have fix a function naming typo in ElasticFusion, which have been merged into mp3guy/ElasticFusion
see:
mp3guy/ElasticFusion#149

modules/labelfusion/rendertrainingimages.py Outdated Show resolved Hide resolved
@@ -34,7 +34,7 @@
lcmlog_filename = dataMap["lcmlog"]

# call ElasticFusion
os.system(path_to_ElasticFusion_executable + " -l ./" + lcmlog_filename)
os.system(path_to_ElasticFusion_executable + " -l ./" + lcmlog_filename + " -cal camera.cfg")
Copy link
Member

Choose a reason for hiding this comment

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

i think this might break existing workflows and data distributions. maybe this script could check if the file exists before adding the -cal argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good.
i will try it later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

setup_environment.sh Outdated Show resolved Hide resolved
@ianchen-tw
Copy link
Contributor Author

hey! @patmarion
It's all good now. just give it a look if you have time.

@akeaveny
Copy link

Just adding a note for changes I had to make to get this to work with a GeForce GTX 1650 & Cuda compilation tools, release 8.0, V8.0.61 (from nvcc -V):

cd ~/ElasticFusion/Core/src/
vim CMakeLists.txt

delete the following line:
set(CUDA_ARCH_BIN "30 35 50 52 61" CACHE STRING "Specify 'real' GPU arch to build binaries for, BIN(PTX) format is supported. Example: 1.3 2.1(1.3) or 13 21(13)")

cd ../
rm -rf build && mkdir build && cd build && cmake ../src/ && make -j20

cd ../../GUI/
rm -rf build && mkdir build && cd build && cmake ../src/ && make -j20

In ElasticFusion build folder run (from sample data):
./ElasticFusion -l ~/labelfusion/data/logs/2017-06-13-29/original_log.lcmlog -f

or from ~/labelfusion/data/logs/2017-06-13-29/:
run_prep

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.

5 participants