use Eigen::Vector3f to improve the clarity of the code (and speed?) #83
Conversation
In the code there was a lot of copy and paste that makes the code hard to read. using Eigen the resulting code is easier to read and, accidentally it might be faster. it is also a very bad idea to add "using namespace std" in a header file...
| @@ -153,6 +154,8 @@ POINT_CLOUD_REGISTER_POINT_STRUCT (PointXYZIRPYT, | |||
| (double, time, time) | |||
| ) | |||
|
|
|||
| typedef Eigen::Vector3f Vector3D; | |||
There was a problem hiding this comment.
I am an old guy... Yes, it is equivalent.
| @@ -38,7 +38,9 @@ class FeatureAssociation{ | |||
|
|
|||
| private: | |||
|
|
|||
| ros::NodeHandle nh; | |||
| enum { SIZE_SCANS = N_SCAN * Horizon_SCAN }; | |||
There was a problem hiding this comment.
In this context, would it be better to use a scoped enum?
There was a problem hiding this comment.
It is already scoped inside the class.
From the "outside", it must be called FeatureAssociation::SIZE_SCANS
Anyway, the whole software has a bunch of global variables that are not scoped in utils.h ;)
| ros::NodeHandle nh; | ||
| enum { SIZE_SCANS = N_SCAN * Horizon_SCAN }; | ||
|
|
||
| ros::NodeHandle nh; |
There was a problem hiding this comment.
Would it make sense to use a reference to the nodehandle instead of creating a new one?
There was a problem hiding this comment.
Most definitively, but it is not the purpose of this PR.
I think it is even more important to achieve a good library / node separation first.
But again, it is not the purpose of this PR
|
The purpose of this code (and the next one I will submit soon) is just to avoid copy and pasting a lot of code because of X,Y,Z and use the more expressive syntax of Eigen (that is a dependency already, because of PCL). There are many way the code can be improved in terms of non-functional design, but it is better to approach this through small PRs |
|
LGTM 👍 |
|
Kind reminder :) any reason to NOT merge this? |
|
Closing this, because this improvement was already added to branch https://github.com/facontidavide/LeGO-LOAM/tree/single_process |
Hi,
first of all, it is great that you shared your work on Github and the results seems very interesting!
In terms of code quality, I believe that there are few possible improvements that can be done to make the code more readable.
With this PR, I propose a change that improve a little bit the code quality using a proper vector3D class instead of 3 individual float values.
Using Eigen the resulting code is easier to read and, accidentally it
might be faster.
it is also a very bad idea to add "using namespace std" in a header
file...
This change is the base to add more code simplifications later (I will submit more PRs), but I prefer to keep it small to make easier for you to review it :)