c42f/displaz

Crash with valid ply file

Closed this issue · 9 comments

c42f commented

The attached ply file (generated by julia bindings) crashes displaz when run as

displaz -shader generic_points.glsl crash.ply

The issue seems to be related to passing a float32 type into a shader variable which should be an integer.

$ xxd crash.ply:

0000000: 706c 790a 666f 726d 6174 2062 696e 6172  ply.format binar
0000010: 795f 6c69 7474 6c65 5f65 6e64 6961 6e20  y_little_endian 
0000020: 312e 300a 636f 6d6d 656e 7420 4469 7370  1.0.comment Disp
0000030: 6c61 7a20 6e61 7469 7665 0a65 6c65 6d65  laz native.eleme
0000040: 6e74 2076 6572 7465 785f 706f 7369 7469  nt vertex_positi
0000050: 6f6e 2031 300a 7072 6f70 6572 7479 2066  on 10.property f
0000060: 6c6f 6174 3634 2078 0a70 726f 7065 7274  loat64 x.propert
0000070: 7920 666c 6f61 7436 3420 790a 7072 6f70  y float64 y.prop
0000080: 6572 7479 2066 6c6f 6174 3634 207a 0a65  erty float64 z.e
0000090: 6c65 6d65 6e74 2076 6572 7465 785f 636f  lement vertex_co
00000a0: 6c6f 7220 3130 0a70 726f 7065 7274 7920  lor 10.property 
00000b0: 666c 6f61 7433 3220 720a 7072 6f70 6572  float32 r.proper
00000c0: 7479 2066 6c6f 6174 3332 2067 0a70 726f  ty float32 g.pro
00000d0: 7065 7274 7920 666c 6f61 7433 3220 620a  perty float32 b.
00000e0: 656c 656d 656e 7420 7665 7274 6578 5f6d  element vertex_m
00000f0: 6172 6b65 7273 697a 6520 3130 0a70 726f  arkersize 10.pro
0000100: 7065 7274 7920 666c 6f61 7433 3220 300a  perty float32 0.
0000110: 656c 656d 656e 7420 7665 7274 6578 5f6d  element vertex_m
0000120: 6172 6b65 7273 6861 7065 2031 300a 7072  arkershape 10.pr
0000130: 6f70 6572 7479 2066 6c6f 6174 3332 2030  operty float32 0
0000140: 0a65 6e64 5f68 6561 6465 720a b176 1478  .end_header..v.x
0000150: 2dce f5bf 018c 8644 aee7 f4bf 0e48 1468  -......D.....H.h
0000160: 6907 e3bf e539 ca33 90eb d9bf e339 54ef  i....9.3.....9T.
0000170: 2d12 d2bf a429 04f9 2dd6 e5bf 6798 b23f  -....)..-...g..?
0000180: 8874 f2bf 7646 4d65 c232 f13f ffba 3de9  .t..vFMe.2.?..=.
0000190: c4de d53f af41 8d35 eb24 ebbf 212f 253f  ...?.A.5.$..!/%?
00001a0: 9080 bdbf 2ef0 5fcb 1240 de3f 3a75 e4ff  ......_..@.?:u..
00001b0: 2134 e83f f049 8f28 219d cb3f cf90 3c2a  !4.?.I.(!..?..<*
00001c0: 28aa 03c0 31c7 4294 ea29 edbf 2751 05e5  (...1.B..)..'Q..
00001d0: deea ed3f 869a 1566 02a1 eabf bd09 d251  ...?...f.......Q
00001e0: b2c2 f13f 0040 8d2a 337d 0540 ca4d fc13  ...?.@.*3}.@.M..
00001f0: 8994 e6bf 22ec 6a5c 7208 ab3f a1ab e652  ....".j\r..?...R
0000200: c364 f2bf 3422 44f8 ba79 e23f 05f6 cdb5  .d..4"D..y.?....
0000210: 66e2 e0bf f33a 7e16 083d f63f 8a00 da0c  f....:~..=.?....
0000220: 3a6e ba3f 2c91 b9f0 ab7e f13f bf2a 5c76  :n.?,....~.?.*\v
0000230: c16a dc3f 4a04 f506 0d85 e93f 0000 803f  .j.?J......?...?
0000240: 0000 803f 0000 803f 0000 803f 0000 803f  ...?...?...?...?
0000250: 0000 803f 0000 803f 0000 803f 0000 803f  ...?...?...?...?
0000260: 0000 803f 0000 803f 0000 803f 0000 803f  ...?...?...?...?
0000270: 0000 803f 0000 803f 0000 803f 0000 803f  ...?...?...?...?
0000280: 0000 803f 0000 803f 0000 803f 0000 803f  ...?...?...?...?
0000290: 0000 803f 0000 803f 0000 803f 0000 803f  ...?...?...?...?
00002a0: 0000 803f 0000 803f 0000 803f 0000 803f  ...?...?...?...?
00002b0: 0000 803f cdcc cc3d cdcc cc3d cdcc cc3d  ...?...=...=...=
00002c0: cdcc cc3d cdcc cc3d cdcc cc3d cdcc cc3d  ...=...=...=...=
00002d0: cdcc cc3d cdcc cc3d cdcc cc3d 0000 803f  ...=...=...=...?
00002e0: 0000 803f 0000 803f 0000 803f 0000 803f  ...?...?...?...?
00002f0: 0000 803f 0000 803f 0000 803f 0000 803f  ...?...?...?...?
0000300: 0000 803f 0a                             ...?.
visr commented

Not sure if related, but I tried Displaz.jl on Windows today (cool stuff!), and it generated invalid ply files. Reason was I copied over the script, which converted LF endings to CRLF. Writing multiline strings to the header then inserted CRLF, whereas the later write statements all end with \n (LF).

The RPly website states that

RPly also accepts files that start with ply\r\n, in which case the end-of-line terminator is assumed to be \r\n throughout.

So the problem was mixed CRLF and LF, after changing

write(fid,
    """
    ply
    format binary_little_endian 1.0
    comment Displaz native
    """
)

to

write(fid, "ply\n")
write(fid, "format binary_little_endian 1.0\n")
write(fid, "comment Displaz native\n")

It worked. However just using the original Displaz.jl with intact LF endings it worked without modification. Still wanted to mention it here since it took some time to find out.

c42f commented

I'm surprised Displaz.jl even works properly on windows since it hasn't really been tested there!

Your problem seems unrelated to the issue above, but thanks for leaving the note. Let me just check - your issue was to do with the way you got Displaz.jl onto your system, rather than a bug in Displaz.jl itself? If there's an actual bug then by all means I'm happy to fix it up.

I'm afraid you'll find the displaz julia bindings are pretty basic at this stage, and not particularly well thought out. There's definitely some more careful thought and design required on my part, and extra features to implement (eg, displaz can show line segments and meshes, I just haven't hooked that up yet.)

visr commented

You're right, it's not directly a bug in Displaz.jl itself, since it comes with LF endings. I was somewhat surprised that multiline strings grab the CRLF and not just LF, because that's what it does in the REPL. This is why this test is the way it is with the nl variable. That being said, for the specific case of writing ply headers, maybe it would be good to use my change posted above, as it is a trap you can easily fall into on Windows.

Offtopic: You may be interested that I started writing https://github.com/visr/LibLAS.jl (also needs work still)

c42f commented

Ah this is very interesting, I'll certainly check out LibLAS.jl when I need to read las from julia. I was toying with the idea of writing a pure julia implementation of a las reader, but I don't think I have the time on my hands to do a good job of it. On the C++ side, both liblas and laslib leave me a bit dissatisfied - they're quite heavyweight beasts if you simply want to read a las file and process it with your own code.

I'm loathe to give up the multi-line strings because they're so darn convenient, though I don't use many of them in Displaz.jl so I'll just swap over to your suggestion. I think the convention of having newlines in files mean something different from "\n" is just confusing and should arguably be changed in julia itself. I'll create an issue on julialang.

visr commented

I agree, thanks for creating the issue.

c42f commented

No worries, I also pushed a patch to master containing your suggested workaround for now. Actually it's less lines of code in this simple case - I can't complain about that :)

c42f commented

This has been fixed for some time (can't remember exactly the commit which fixed it...)

visr commented

And so is string normalization, so don't hold back on the multiline stings :-)

I was toying with the idea of writing a pure julia implementation of a las reader

Just wanted to point out (WIP) https://github.com/visr/LasIO.jl
Feedback welcome!

c42f commented

Great, thanks!