Oslandia / py3dtiles

:warning: Project migrated to : https://gitlab.com/py3dtiles/py3dtiles :warning:
https://py3dtiles.org
Other
213 stars 77 forks source link

convert colored points from .xyz files #54

Closed delhomer closed 5 years ago

delhomer commented 5 years ago

This PR aims at fixing the .xyz file conversion when they contain RGB values.

Contrary to the .las files, there is no metadata for describing file features. We need to pass the write_rgb variable to xyz_reader.run function in order to manage this case. I chose to pass it to las_reader.run as well, so as to keep the signatures equal (however the parameter is dummy with .las...).

The PR fixes #53.

autra commented 5 years ago

Thanks @delhomer!

Contrary to the .las files, there is no metadata for describing file features. We need to pass the write_rgb variable to xyz_reader.run function in order to manage this case.

If I'm not mistaken, write_rgb is a way to configure the output, not a way to deal with missing metadata. We need something else to specify how the xyz file is made (because this is not a standard format).

Maybe we can start by supporting the fme interpretation of xyz only? (so XYZIRGB for a start). It seems to be the same format as CloudCompare?

If we need more flexibility, we could have a cli option for that. WDYT?

delhomer commented 5 years ago

If I'm not mistaken, write_rgb is a way to configure the output, not a way to deal with missing metadata.

You probably don't! I believed that --rgb did the job, however you're right; I read the doc far too quickly (this argument is passed to the zmq process for setting write_rgb).

We need something else to specify how the xyz file is made (because this is not a standard format).

Exactly, and that is the starting point of my proposal: the current code is only able to manage XYZ columns; that being a problem when handling colored points.

Maybe we can start by supporting the fme interpretation of xyz only? (so XYZIRGB for a start). It seems to be the same format as CloudCompare?

That sounds like a good deal. As a consequence, do we consider .xyz files with less than 7 columns as follows:

Or alternatively, do we raise an exception if the input data has wrong dimensions (quite a violent design, isn't it?)?

If we need more flexibility, we could have a cli option for that. WDYT?

Well... --be-kind = accept less-than-7-column files? :)

autra commented 5 years ago
3 columns : only XYZ, we set I, R, G, B to 255
4 columns : XYZ+I, we set R, G, B to 255
6 columns : XYZRGB, we set I to 255

In my opinion we shouldn't invent missing data and let the viewer decides how it wants to display them (it's its job, isn't it?). With itowns, you can give a a color to a pointcloud for instance.

so just set what you have, and leave the rest unset. Sounds good for you?

autra commented 5 years ago

Or alternatively, do we raise an exception if the input data has wrong dimensions (quite a violent design, isn't it?)?

How would you define wrong here? We don't really know what's wrong or right concerning xyz, let's be inclusive ;-)

By flexibility, I meant "accept non classic order of columns". But let's not care for that now.

delhomer commented 5 years ago

so just set what you have, and leave the rest unset. Sounds good for you?

Deal. :)

delhomer commented 5 years ago

How would you define wrong here? We don't really know what's wrong or right concerning xyz, let's be inclusive ;-)

By flexibility, I meant "accept non classic order of columns". But let's not care for that now.

Here the point is that we can't be sure of the xyz file content, especially when the number of columns is unusual (or, let say, different from 7). I will first consider the above hypothesis by leaving the unknown features unset, as said in your last comment.

Commits in preparation!

autra commented 5 years ago

Good for me!