Add Point cloud loaders (LAS and TXT) #978

Marina2468 · 2022-07-26T07:44:07Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Be aware that the PR request cannot be accepted if it doesn't pass the Continuous Integration tests.

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature
What is the current behavior? (You can also link to an open issue here)
no support in radium to load .las and .txt point clouds
What is the new behavior (if this is a feature change)?
add file loaders for .las and .txt file formats
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
no
Other information:

for more information, see https://pre-commit.ci

src/IO/filelist.cmake

for more information, see https://pre-commit.ci

codecov · 2022-07-26T08:17:19Z

Codecov Report

Merging #978 (4fc2c2b) into release-candidate (edd1179) will decrease coverage by 0.37%.
The diff coverage is 0.00%.

@@                  Coverage Diff                  @@
##           release-candidate     #978      +/-   ##
=====================================================
- Coverage              39.93%   39.56%   -0.38%     
=====================================================
  Files                    345      349       +4     
  Lines                  22928    23146     +218     
=====================================================
  Hits                    9157     9157              
- Misses                 13771    13989     +218

Impacted Files	Coverage Δ
src/IO/AsciiPCLoader/asciipcloader.cpp	`0.00% <0.00%> (ø)`
src/IO/AsciiPCLoader/asciipcloader.hpp	`0.00% <0.00%> (ø)`
src/IO/LasLoader/lasloader.cpp	`0.00% <0.00%> (ø)`
src/IO/LasLoader/lasloader.hpp	`0.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us.

nmellado

First round of reviews

src/IO/AsciiPCLoader/asciipcloader.cpp

nmellado · 2022-07-26T09:22:57Z

src/IO/AsciiPCLoader/asciipcloader.cpp

+    ypos++;
+    zpos++;
+
+    /*retrieving x y z time attribut positions and column count*/


With this implementation, your loader can only load the x,y,z and time fields.
I would be better to load all the fields, then aggregate x,y,z, and add the position, and custom fields in the Radium datastructure.

updated implementation to load all attributes as custom and aggregate x y z properties only

src/IO/LasLoader/lasloader.cpp

nmellado · 2022-07-26T09:30:05Z

src/IO/LasLoader/lasloader.cpp

+
+    // checking for colors
+    bool colors       = false;
+    auto handle_color = geometry->getGeometry().vertexAttribs().addAttrib<Ra::Core::Vector4>(


Check if colors exist in file before adding them as vertexAttrib

Same for gps info

if they are not in file, i delete them at the end (lines 220-227)

This looks sub-optimal.
Can't you just move the attrib creation after the check ?

in that case I would need a temporary vector to store colors and gps_time in the data reading loop, and that is what we tried to avoid in the first place

I don't think so.
You can call addAttrib at line 137, ie. when setting colors to true.
In fact you don't need the colors variable: you can only check if handle_color is valid, using
handle_color.idx().isValid()

See https://storm-irit.github.io/Radium-Engine/release-candidate/classRa_1_1Core_1_1Utils_1_1AttribHandle.html#af86250020bb2831ce80a811c17d3a6c5
and https://storm-irit.github.io/Radium-Engine/release-candidate/Index_8hpp_source.html

updated code to call findAttrib every iteration, please check if it's the suitable behaviour (i did not use idx()...)

Co-authored-by: Nicolas Mellado <nmellado0@gmail.com>

…pcloaders

for more information, see https://pre-commit.ci

Co-authored-by: Nicolas Mellado <nmellado0@gmail.com>

for more information, see https://pre-commit.ci

nmellado · 2022-07-26T13:16:38Z

src/IO/LasLoader/lasloader.cpp

+    bool colors = false;
+    if ( data_format == 2 || data_format == 3 || ( minor >= 3 && data_format == 5 ) ) {
+        auto handle_color = geometry->getGeometry().vertexAttribs().addAttrib<Ra::Core::Vector4>(
+            Ra::Core::Geometry::getAttribName( Ra::Core::Geometry::MeshAttrib::VERTEX_COLOR ) );
+        colors = true;


Suggested change

bool colors = false;

if ( data_format == 2 || data_format == 3 || ( minor >= 3 && data_format == 5 ) ) {

auto handle_color = geometry->getGeometry().vertexAttribs().addAttrib<Ra::Core::Vector4>(

Ra::Core::Geometry::getAttribName( Ra::Core::Geometry::MeshAttrib::VERTEX_COLOR ) );

colors = true;

Ra::Core::Utils::AttribHandle< Ra::Core::Vector4 > handle_color;

if ( data_format == 2 || data_format == 3 || ( minor >= 3 && data_format == 5 ) ) {

handle_color = geometry->getGeometry().vertexAttribs().addAttrib<Ra::Core::Vector4>(

Ra::Core::Geometry::getAttribName( Ra::Core::Geometry::MeshAttrib::VERTEX_COLOR ) );

nmellado · 2022-07-26T13:16:58Z

src/IO/LasLoader/lasloader.cpp

+        vertices.emplace_back( Scalar( x ), Scalar( y ), Scalar( z ) );
+
+        // computing colors if found
+        if ( colors ) {


Suggested change

if ( colors ) {

if ( handle_colors.idx().isValid() ) {

nmellado · 2022-07-26T13:17:14Z

src/IO/LasLoader/lasloader.cpp

+            auto handle_colors =
+                geometry->getGeometry().vertexAttribs().findAttrib<Ra::Core::Vector4>(
+                    Ra::Core::Geometry::getAttribName(
+                        Ra::Core::Geometry::MeshAttrib::VERTEX_COLOR ) );


Suggested change

auto handle_colors =

geometry->getGeometry().vertexAttribs().findAttrib<Ra::Core::Vector4>(

Ra::Core::Geometry::getAttribName(

Ra::Core::Geometry::MeshAttrib::VERTEX_COLOR ) );

nmellado · 2022-07-26T13:22:57Z

src/IO/LasLoader/lasloader.cpp

+            geometry->getGeometry()
+                .vertexAttribs()
+                .getDataWithLock( handle_colors )
+                .emplace_back( Scalar( red ), Scalar( green ), Scalar( blue ), 1_ra );
+            geometry->getGeometry().vertexAttribs().unlock( handle_colors );


It will be more efficient to look for the attrib only once.
Note that you can do

auto& attr = geometry->getGeometry(). getAttrib( handle_colors );

I think that I now understand why you need your temporary arrays, my bad.
So:

use temporary std::vector for colors (and other attributes)

after reading, copy them using geometry->getGeometry(). getAttrib( handle_colors ).setData().

for more information, see https://pre-commit.ci

src/IO/AsciiPCLoader/asciipcloader.cpp

nmellado · 2022-07-27T08:02:05Z

src/IO/AsciiPCLoader/asciipcloader.cpp

+             ( (int)i == timepos ) ) {
+            continue;
+        }
+        auto handle2 = geometry->getGeometry().vertexAttribs().findAttrib<Scalar>( tokens[i] );


I guess it would be better to store the handle in an array, it would be easier to lock/unlock.

nmellado · 2022-07-27T08:03:28Z

src/IO/AsciiPCLoader/asciipcloader.hpp

+     *
+     * example: Time X Y Z Roll Pitch Heading sdX sdY sdZ
+     *
+     * X, Y, Z and Time properties (case sensitive) are mandatory


I am wondering if we could relax the constraint on the time field: it could be useful to load point-clouds with xyz coordinates, even if we don't have the time field.

done. now only position properties are mandatory

nmellado · 2022-07-27T08:06:13Z

src/IO/LasLoader/lasloader.cpp

+                blue  = *(unsigned short*)( point.data() + 24 );
+            }
+
+            colors.emplace_back( Scalar( red ), Scalar( green ), Scalar( blue ), 1_ra );


As you can see here https://storm-irit.github.io/Radium-Engine/release-candidate/classRa_1_1Core_1_1Utils_1_1ColorBase.html
Colors have values between 0 and 1.
As you cast unsigned short to float, you will always have values > 1.

so do I need to perform a division by 2¹⁶?

You need to be sure what is the maximum value in practice. Usually it is 255.

it says in the specification that color channels are coded on 16 bits and that it should always be normalized to be so

src/IO/LasLoader/lasloader.hpp

for more information, see https://pre-commit.ci

Marina2468 and others added 3 commits July 25, 2022 10:09

add loaders for las and txt file format (point clouds)

df9d559

fix typo

9eccdfb

[pre-commit.ci] auto fixes from pre-commit.com hooks

afaa7d9

for more information, see https://pre-commit.ci

nmellado added enhancement Type of issue/PR: enhancement IO Related to Ra::IO labels Jul 26, 2022

nmellado changed the title ~~Pcloaders~~ Add Point cloud loaders (LAS and TXT) Jul 26, 2022

nmellado reviewed Jul 26, 2022

View reviewed changes

src/IO/filelist.cmake Outdated Show resolved Hide resolved

Marina2468 and others added 2 commits July 26, 2022 10:02

remove compilation guards

5e396dd

[pre-commit.ci] auto fixes from pre-commit.com hooks

faa0394

for more information, see https://pre-commit.ci

nmellado self-assigned this Jul 26, 2022

nmellado reviewed Jul 26, 2022

View reviewed changes

Marina2468 and others added 13 commits July 26, 2022 12:10

Update src/IO/AsciiPCLoader/asciipcloader.cpp

b478589

Co-authored-by: Nicolas Mellado <nmellado0@gmail.com>

constructors set to default

9e979c3

Merge branch 'pcloaders' of github.com:Marina2468/Radium-Engine into …

d171d97

…pcloaders

[pre-commit.ci] auto fixes from pre-commit.com hooks

3573b19

for more information, see https://pre-commit.ci

Update src/IO/AsciiPCLoader/asciipcloader.cpp

814211f

Co-authored-by: Nicolas Mellado <nmellado0@gmail.com>

Update src/IO/AsciiPCLoader/asciipcloader.cpp

bbf1404

Co-authored-by: Nicolas Mellado <nmellado0@gmail.com>

destructors set to default

1e49cd1

remove malloc

2c53183

[pre-commit.ci] auto fixes from pre-commit.com hooks

211b6f6

for more information, see https://pre-commit.ci

group log (remove code duplicate)

c16169b

[pre-commit.ci] auto fixes from pre-commit.com hooks

85c7236

for more information, see https://pre-commit.ci

update color and gps handling

5b6f991

[pre-commit.ci] auto fixes from pre-commit.com hooks

6f9e1fa

for more information, see https://pre-commit.ci

nmellado reviewed Jul 26, 2022

View reviewed changes

Marina2468 and others added 4 commits July 26, 2022 21:56

update to load all attribs as custom

c21c520

[pre-commit.ci] auto fixes from pre-commit.com hooks

c06f7a0

for more information, see https://pre-commit.ci

update las loader

c974425

[pre-commit.ci] auto fixes from pre-commit.com hooks

769a183

for more information, see https://pre-commit.ci

nmellado reviewed Jul 27, 2022

View reviewed changes

Marina2468 and others added 9 commits July 27, 2022 12:34

fix custom attribs reference issue

2a258b8

fix colors range

4b470bd

[pre-commit.ci] auto fixes from pre-commit.com hooks

2f6cfde

for more information, see https://pre-commit.ci

remove properties' log

1ded72e

Merge branch 'release-candidate' into pcloaders

b361d35

add guards on read + improve char* to double cast

d830798

[pre-commit.ci] auto fixes from pre-commit.com hooks

484848f

for more information, see https://pre-commit.ci

initialize to avoid warning

c30f473

[pre-commit.ci] auto fixes from pre-commit.com hooks

4fc2c2b

for more information, see https://pre-commit.ci

dlyr marked this pull request as draft January 18, 2023 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Point cloud loaders (LAS and TXT) #978

Add Point cloud loaders (LAS and TXT) #978

Marina2468 commented Jul 26, 2022

codecov bot commented Jul 26, 2022 •

edited

Loading

nmellado left a comment

nmellado Jul 26, 2022

Marina2468 Jul 26, 2022

nmellado Jul 26, 2022

nmellado Jul 26, 2022

Marina2468 Jul 26, 2022

nmellado Jul 26, 2022

Marina2468 Jul 26, 2022

nmellado Jul 26, 2022 •

edited

Loading

Marina2468 Jul 26, 2022

nmellado Jul 26, 2022

nmellado Jul 26, 2022

nmellado Jul 26, 2022

nmellado Jul 26, 2022

Marina2468 Jul 27, 2022

nmellado Jul 27, 2022

Marina2468 Jul 27, 2022

nmellado Jul 27, 2022

Marina2468 Jul 27, 2022

nmellado Jul 27, 2022

Marina2468 Jul 27, 2022

nmellado Jul 27, 2022

Marina2468 Jul 27, 2022

Add Point cloud loaders (LAS and TXT) #978

Are you sure you want to change the base?

Add Point cloud loaders (LAS and TXT) #978

Conversation

Marina2468 commented Jul 26, 2022

codecov bot commented Jul 26, 2022 • edited Loading

Codecov Report

nmellado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmellado Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2022 •

edited

Loading

nmellado Jul 26, 2022 •

edited

Loading