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

Use specific components from PCL. Add Drake camera simulation. #76

Conversation

EricCousineau-TRI
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI commented Jan 5, 2018

This is a simple test in relation to the discussion in RobotLocomotion/drake#7702.

\cc @RussTedrake


This change is Reviewable

@EricCousineau-TRI
Copy link
Collaborator Author

Currently segfaults when destructing, not sure why:
https://gist.github.com/EricCousineau-TRI/2bc264d2532b6bf0e01676edf9f32af0

@jwnimmer-tri
Copy link
Contributor

That looks a lot like the RobotLocomotion/drake#7935 failure mode with a hidden display?

@jamiesnape
Copy link
Contributor

That looks a lot like the RobotLocomotion/drake#7935 failure mode with a hidden display?

RobotLocomotion/drake#7908, actually, I think my comment was misinterpreted and we ended up with duplicate issues.

@jwnimmer-tri
Copy link
Contributor

Agreed.

@jamiesnape
Copy link
Contributor

I think I would prefer to split test_pcl and test_drake_camera into separate files.


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


drake_cmake_installed/src/pcl/simple_pcl_example.cc, line 44 at r1 (raw file):

#include <iostream>
#include <random>
#include <memory>

Alphabetize


Comments from Reviewable

@EricCousineau-TRI
Copy link
Collaborator Author

Is there a reason why?

I'm trying to think of a good split that shows both a simple example and still tests what I want; I've split it into to test_pcl and test_drake_camera, and it feels weird to either (a) duplicate test_pcl between this and simple_pcl_example or (b) include test_pcl.h and call it in simple_pcl_example - main and something like test_drake_and_pcl - main.


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Collaborator Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


drake_cmake_installed/src/pcl/simple_pcl_example.cc, line 44 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Alphabetize

Done.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Maybe I am missing something, but I do not see how the two examples are connected. test_drake_camera does not even appear to use PCL.


Review status: 1 of 2 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Collaborator Author

I had an apparently irrational superstition that the binary had be linked with code that consumes PCL to ensure that the linking does not get optimized out. Realized that was ill-founded, so I've split 'em. Sorry about that!


Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Feb 9, 2018

Rebased #90 on top of this revision. It still fails the same way on my system.

@jamiesnape
Copy link
Contributor

:lgtm:


Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


drake_cmake_installed/src/pcl/test_drake_camera.cc, line 2 at r2 (raw file):

/*****************************************************************************
 * Copyright (c) 2017, Toyota Research Institute.

BTW 2018


Comments from Reviewable

@EricCousineau-TRI
Copy link
Collaborator Author

Uh oh - it looks like CircleCI also suffers from the XVFB thing... Is there any option to enable this, or should we just punt on this being a ctest?

@EricCousineau-TRI
Copy link
Collaborator Author

Punting on add_test for test_drake_camera -- however, it looks like TravisCI does test it successfully, so I may briefly look into enabling this only on Travis.

@jamiesnape
Copy link
Contributor

I can get it working on CircleCI easy enough. It will be later today, though.

@jamiesnape
Copy link
Contributor

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


drake_cmake_installed/src/pcl/CMakeLists.txt, line 54 at r3 (raw file):

add_executable(test_drake_camera test_drake_camera.cc)
target_link_libraries(test_drake_camera drake::drake ${PCL_LIBRARIES})

So this is supposed to be a repo of exemplary uses of Drake and unused PCL linker flags are not really exemplary. If we really want to test whether the linker flag is an issue then there should be some meaningful usage of PCL within that test (not just unrelated code in the same file).


Comments from Reviewable

@EricCousineau-TRI
Copy link
Collaborator Author

Review status: 1 of 6 files reviewed at latest revision, 2 unresolved discussions.


drake_cmake_installed/src/pcl/CMakeLists.txt, line 54 at r3 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

So this is supposed to be a repo of exemplary uses of Drake and unused PCL linker flags are not really exemplary. If we really want to test whether the linker flag is an issue then there should be some meaningful usage of PCL within that test (not just unrelated code in the same file).

I've added a TODO to the example file. Is that sufficient for now?


drake_cmake_installed/src/pcl/test_drake_camera.cc, line 2 at r2 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

BTW 2018

Done.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Collaborator Author

if you would like non-trivial code PCL code with Drake, I'm tinkering with it now - but I could always submit it as a chaser PR :)

@jamiesnape
Copy link
Contributor

A follow-up PR would be good.


Reviewed 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


drake_cmake_installed/CMakeLists.txt, line 36 at r4 (raw file):

project(drake_cmake_installed)

set(RUN_X11_TESTS OFF CACHE BOOL "Run tests which require X11")

option(RUN_X11_TESTS "Run tests that require X11" OFF), though the tests don't require X11 on Mac per se, but I guess this is fine (it is temporary anyway).


drake_cmake_installed/src/pcl/CMakeLists.txt, line 54 at r3 (raw file):

Previously, EricCousineau-TRI wrote…

I've added a TODO to the example file. Is that sufficient for now?

Yes.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Collaborator Author

Review status: 2 of 6 files reviewed at latest revision, 1 unresolved discussion.


drake_cmake_installed/CMakeLists.txt, line 36 at r4 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

option(RUN_X11_TESTS "Run tests that require X11" OFF), though the tests don't require X11 on Mac per se, but I guess this is fine (it is temporary anyway).

Done.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Reviewed 4 of 4 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@EricCousineau-TRI EricCousineau-TRI merged commit a708bc2 into RobotLocomotion:master Feb 9, 2018
@jamiesnape jamiesnape removed their request for review June 17, 2021 15:11
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.

None yet

3 participants