daavoo / pyntcloud

pyntcloud is a Python library for working with 3D point clouds.
http://pyntcloud.readthedocs.io
MIT License
1.42k stars 224 forks source link

Add support for `bool` type in PLY files #309

Closed Nicholas-Autio-Mitchell closed 2 years ago

Nicholas-Autio-Mitchell commented 3 years ago

Is your feature request related to a problem? Please describe.

PyntCloud.from_file() fails if the PLY contains a bool type, which occurs here in io/ply.py.

This is because there is no mapping for bool types in the ply_dtypes dictionary:

ply_dtypes = dict([
    (b'bool', '?')       # feature request to simply add this line
    (b'int8', 'i1'),
    (b'char', 'i1'),
    (b'uint8', 'u1'),
    (b'uchar', 'b1'),
    (b'uchar', 'u1'),
    (b'int16', 'i2'),
    (b'short', 'i2'),
    (b'uint16', 'u2'),
    (b'ushort', 'u2'),
    (b'int32', 'i4'),
    (b'int', 'i4'),
    (b'uint32', 'u4'),
    (b'uint', 'u4'),
    (b'float32', 'f4'),
    (b'float', 'f4'),
    (b'float64', 'f8'),
    (b'double', 'f8')
])

The mapping of '?' to bool is shown here under "one-character strings".

Example:

In [1]: import numpy as np

In [2]: np.dtype('?')
Out[3]: dtype('bool')

Describe alternatives you've considered Finding an alternative to using pyntcloud for PLY files 🤷

Additional context bool is a known type for NumPy of course, so it makes sense to support it, as the basic form of PyntCloud objects holds np.ndarrays. It is not specifically mentioned in the original PLY specification (as far as I can see), but supporting PLY files that contain bool fields wouldn't break anything from a user's perspective.

daavoo commented 3 years ago

I guess that the main issue here would be that pyntcloud could be potentially reading/writing PLY files that are not valid in any other point cloud software (or at least not those following the official PLY specification).

Probably using npz format would be the way to go when looking for full NumPy compatibility.

Nicholas-Autio-Mitchell commented 3 years ago

You could also just add an argument like allow_extra_types with the default to False. The joys of python :)

The reason I requested this is because I have see bool values inside PLY files, which didn't come from pyntcloud. On 9 Nov 2021, 16:01 +0100, David de la Iglesia Castro @.***>, wrote:

I guess that the main issue here would be that pyntcloud could be potentially reading/writing PLY files that are not valid in any other point cloud software (or at least not those following the official PLY specification). Probably using npz format would be the way to go when looking for full NumPy compatibility. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

Nicholas-Autio-Mitchell commented 2 years ago

@daavoo - if you don't want to include this feature (behind a flag or otherwise), shall we close this issue?

daavoo commented 2 years ago

@daavoo - if you don't want to include this feature (behind a flag or otherwise), shall we close this issue?

I have no strong opinion @Nicholas-Mitchell . If someone sends P.R. and makes sense, (i.e. doesn't break default behavior) I would happily merge

Nicholas-Autio-Mitchell commented 2 years ago

@daavoo I have opened a PR: https://github.com/daavoo/pyntcloud/pull/321

Will close this issue and continue any discussion in the PR.