Closed 2scholz closed 6 years ago
@vrabaud was concerned about the performance of the previous approach that created a PointCloud1 directly. If we create namedtuples from the results returned by the generator wouldn't this affect the performance in the same way? Apart from this it clearly makes sense not to duplicate the logic.
How should we proceed?
I'm not worried about performance as this is explicitly a lower performance approach than the generator. I just want that to be clearly documented for the users so they don't use the lower performance one without knowing the difference.
And since this code block and the one above are line for line identical except for appending vs yielding they should be collapsed.
I completely agree on the documentation.
It is not true though that both code blocks are identical except for the appending vs yielding. The new function uses named tuples, while the other function yields a list.
For comparison, this is the line in read_points
:
p = unpack_from(data, (row_step * v) + (point_step * u))
And this is the line in read_points_list
:
point = Point._make(unpack_from(data, (row_step * v) + (point_step * u)
I just want to clear any misconceptions, so if you still think the generator is the way to go I will make the necessary changes.
I made a new pull request for the alternative approach you suggested.
Replaced by #112
Add new function
read_points_list
. In contrast toread_points
it returns a list of named tuples, instead of a generator. Using lists one can access the points via index, which is not possible using a generator. The named tuples are indexed by the field name.We use this package in an undergraduate course at our university. Many students struggled using the generator and lists are more intuitive for beginners. For a previous discussion regarding this issue see this pull request.
We would like to see this change in indigo-devel branch as well. This should not be a problem because API did not change.
We used the code below to test on our turtlebots:
@vrabaud Please have a look.