Skip to content

Conversation

mjcarroll
Copy link
Contributor

  • Use threads to accelerate computation (distributing the space equally
    across the threads)
  • Split visiblity functionality into library to deduplicate compilation
  • New validate_visibility_table cc file for main function
  • Use FCL's distance function to disambiguate overlapped tile segments

Signed-off-by: Michael Carroll [email protected]

@nkoenig nkoenig requested a review from caguero September 15, 2020 21:18
@mjcarroll mjcarroll marked this pull request as ready for review September 16, 2020 20:21
@mjcarroll
Copy link
Contributor Author

Note, to test, you will need fcl and libccd. I opted to build these from scratch locally.

repositories:
  fcl:
    type: git
    url: https://github.com/flexible-collision-library/fcl
    version: 0d98b836d57b1a4f457fac8e52d6b22b56e5fdbf
  libccd:
    type: git
    url: https://github.com/danfis/libccd
    version: 7931e764a19ef6b21b443376c699bbc9c6d4fba8

@nkoenig
Copy link
Contributor

nkoenig commented Sep 17, 2020

This targets the citadel_tile_overlap branch, is that correct?

@caguero
Copy link
Contributor

caguero commented Sep 17, 2020

This targets the citadel_tile_overlap branch, is that correct?

Ideally, this should target comms_v2. Additionally, I'm cherry-picking all comms changes to comms_v2_citadel.

@mjcarroll
Copy link
Contributor Author

@caguero friendly ping

@nkoenig
Copy link
Contributor

nkoenig commented Sep 21, 2020

@mjcarroll , can you update this to target comms_v2 instead?

* Use threads to accelerate computation (distributing the space equally
across the threads)
* Split visiblity functionality into library to deduplicate compilation
* New validate_visibility_table cc file for main function
* Use FCL's distance function to disambiguate overlapped tile segments

Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll force-pushed the mjcarroll/citadel_tile_overlap branch from cdf382a to 1d3722e Compare September 21, 2020 16:16
@mjcarroll mjcarroll changed the base branch from citadel_tile_overlap to comms_v2 September 21, 2020 16:16
@mjcarroll
Copy link
Contributor Author

@mjcarroll , can you update this to target comms_v2 instead?

Done.

@caguero
Copy link
Contributor

caguero commented Sep 21, 2020

I just installed libfcl-dev on Ubuntu 18.04 and I don't think it comes with a config.cmake file. Is this expected? See the list of files included in the .deb here.

@mjcarroll
Copy link
Contributor Author

Try using ros-melodic-fcl-catkin, which is a catkinized version. The package.xml and CMakeLists need to be updated accordingly.

private: std::vector<
std::pair<ignition::math::AxisAlignedBox, uint64_t>> worldSegments;

private: std::map<uint64_t, std::string> worldSegmentNames;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Missing doc.


/// \brief A map that stores 3D points an the vertex id in which are located
private: std::map<std::tuple<int32_t, int32_t, int32_t>, uint64_t> vertices;
public : std::map<std::tuple<int32_t, int32_t, int32_t>, uint64_t> vertices;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking, is this change on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was unintentionally left in as part of threading.

@caguero
Copy link
Contributor

caguero commented Sep 21, 2020

Try using ros-melodic-fcl-catkin, which is a catkinized version. The package.xml and CMakeLists need to be updated accordingly.

I can find the cmake.config with this package. I searched and replaced some CMake variables in CMakeLists.txt (using fcl_catkin prefix) but I still see some compilation problems not finding the right headers. Do you mind to update the code?

@mjcarroll
Copy link
Contributor Author

I take it back, after evaluating both upstream fcl and catkin_fcl, they both seem to have some issues. You should use the repos file that I posted above.

Signed-off-by: Michael Carroll <[email protected]>
@nkoenig
Copy link
Contributor

nkoenig commented Sep 22, 2020

I'm getting these compilation errors:

In file included from /usr/include/fcl/BV/RSS.h:41,
                 from /usr/include/fcl/ccd/motion_base.h:46,
                 from /usr/include/fcl/collision_object.h:45,
                 from /home/nkoenig/subt_ws/src/subt/subt_ign/src/ign_to_fcl.hh:4,
                 from /home/nkoenig/subt_ws/src/subt/subt_ign/src/ign_to_fcl.cc:1:
/home/nkoenig/local/include/fcl/math/constants.h:131:1: error: expected primary-expression before ‘typedef’
 typedef typename detail::ScalarTrait<S>::type Real;
 ^~~~~~~
/home/nkoenig/local/include/fcl/math/constants.h:131:1: error: expected ‘}’ before ‘typedef’
/home/nkoenig/local/include/fcl/math/constants.h:130:1: note: to match this ‘{’
 {
 ^
/home/nkoenig/local/include/fcl/math/constants.h:130:2: error: expected ‘;’ before ‘typedef’

I think I've installed the correct version of FCL.

@caguero
Copy link
Contributor

caguero commented Sep 22, 2020

I'm getting these compilation errors:

In file included from /usr/include/fcl/BV/RSS.h:41,
                 from /usr/include/fcl/ccd/motion_base.h:46,
                 from /usr/include/fcl/collision_object.h:45,
                 from /home/nkoenig/subt_ws/src/subt/subt_ign/src/ign_to_fcl.hh:4,
                 from /home/nkoenig/subt_ws/src/subt/subt_ign/src/ign_to_fcl.cc:1:
/home/nkoenig/local/include/fcl/math/constants.h:131:1: error: expected primary-expression before ‘typedef’
 typedef typename detail::ScalarTrait<S>::type Real;
 ^~~~~~~
/home/nkoenig/local/include/fcl/math/constants.h:131:1: error: expected ‘}’ before ‘typedef’
/home/nkoenig/local/include/fcl/math/constants.h:130:1: note: to match this ‘{’
 {
 ^
/home/nkoenig/local/include/fcl/math/constants.h:130:2: error: expected ‘;’ before ‘typedef’

I think I've installed the correct version of FCL.

Michael tweaked the code and now it requires to install FCL from debs (libfcl-dev).

@caguero
Copy link
Contributor

caguero commented Sep 22, 2020

I left two minor comments, ready to approve as the test dat seems to be fine.

@mjcarroll
Copy link
Contributor Author

@nkoenig FCL from debs should be sufficient (libfcl-dev + libccd-dev)

Signed-off-by: Michael Carroll <[email protected]>
@caguero caguero merged commit cccf0d9 into comms_v2 Sep 23, 2020
caguero added a commit that referenced this pull request Sep 23, 2020
* Updated comms model

Signed-off-by: Carlos Agüero <[email protected]>

* Tweak scaling factor and add last breadcrumb for range computations.

Signed-off-by: Carlos Agüero <[email protected]>

* Update subt-communication/subt_rf_interface/include/subt_rf_interface/subt_rf_model.h

* Tweak doc

Signed-off-by: Carlos Agüero <[email protected]>

* Visibility table enhancements (#609)

* Visibility table enhancements

* Use threads to accelerate computation (distributing the space equally
across the threads)
* Split visiblity functionality into library to deduplicate compilation
* New validate_visibility_table cc file for main function
* Use FCL's distance function to disambiguate overlapped tile segments

Signed-off-by: Michael Carroll <[email protected]>

* Cleanups

Signed-off-by: Michael Carroll <[email protected]>

* Adjust to Ubuntu libfcl (0.5.0)

Signed-off-by: Michael Carroll <[email protected]>

* Address reviewer feedback

Signed-off-by: Michael Carroll <[email protected]>

* Adding extra dependencies to Docker

Signed-off-by: Carlos Agüero <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
@nkoenig nkoenig deleted the mjcarroll/citadel_tile_overlap branch December 10, 2020 22:34
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.

3 participants