Add support for other data types in potree pointclouds #767

autra · 2018-05-28T15:42:53Z

Description

We were only supporting position and rgb informations from potree converted pointclouds. This PR aims at supporting all of them. At least I wanted to have them parsed in attributes, but I also added some default rendering to check that it is working correctly.

Some remarks:

I didn't test the oct16 normals, because I don't have any las with normal information or already converted pointclouds with oct16 format for normals. If some of you have one, I'd be very interested :-)
what to do with classification remains to be decided. I could add some default rendering, but I'm not sure which type :-)

autra · 2018-05-28T15:43:29Z

src/Parser/PotreeBinParser.js

+    },
+    // Note: at the time of writing, PotreeConverter will only generate normals in Oct16 format
+    // see PotreeConverter.cpp:121
+    // keeping all the values to stay iso to PointAttributes.cpp


Actually keeping all the values to be able to read clouds converted with old version. I'm editing that.

mbredif

I left some comments. Do you get better performance than using an interleavedarraybuffer (assuming it is possible) ?

mbredif · 2018-05-28T16:17:39Z

src/Parser/PotreeBinParser.js

+                            attr.array[pntIdx * attr.numElements + 2]);
+                    tightbbox.min.min(tmp);
+                    tightbbox.max.max(tmp);
+                }


would not it be simpler to use geometry.computeBoundingBox() ?

Indeed. I've checked the implementation, and it is essentially the same as here. It'll add a for loop, but I don't expect its cost to be perceivable.

mbredif · 2018-05-28T16:20:49Z

src/Process/PointCloudProcessing.js

@@ -165,6 +165,7 @@ export default {
            if (elt.obj) {
                elt.obj.material.visible = true;
                elt.obj.material.size = layer.pointSize;
+                elt.obj.material.uniforms.mode.value = layer.mode;


that would break using a material with no mode uniform.
you could instead set here elt.obj.material.mode = layer.mode; and update the uniform inside the updateUniforms function.

Actually, what about passing layer to updateUniforms? That's more or less the pattern we used in iTowns: update uniforms according to some config values attached to layer. It'll make it explicit, and would avoid yet another value copy (they are getting difficult to follow).

mbredif · 2018-05-28T16:30:52Z

what to do with classification remains to be decided. I could add some default rendering, but I'm not sure which type :-)

two ideas:

simplest thing : you can use an overlaycolor indexed by the classification Id.
much more interesting and involved approach : this paper :)

autra · 2018-05-29T09:55:27Z

Do you get better performance than using an interleavedarraybuffer (assuming it is possible) ?

I just didn't think about using it :-) Thanks for the idea, I'm gonna try.

what to do with classification remains to be decided. I could add some default rendering, but I'm not sure which type :-)

two ideas:

simplest thing : you can use an overlaycolor indexed by the classification Id.

much more interesting and involved approach : this paper :)

Interesting paper!

For a basic render for testing purpose, I'd prefer to have something simpler (this paper technique necessitates a special pre-processing (so not all converted pointclouds can be displayed with it) and multipass rendering). I was thinking of doing exactly what your first proposal is, but couldn't decide if this was worth it here or not. After all, once you have the classification in the shader, applying some color is very easy and we could let app developers decide how they want to render it, because they'll know their classification, the number of them etc.? As I was not sure, I thought it better to leave it out for now and add it later once our mind is made, WDYT?

Btw, I didn't upload a screenshot of normal rendering!

This pointcloud is lion_takanawa present in the potree repository. I'll look into the licence to see if we can add it to itowns-sample-data (and get the las, that'll be great to test oct16 normal rendering).

autra · 2018-05-29T15:09:11Z

@mbredif concerning InterleavedBuffer, the way they are implemented in THREE.js seems to lack an important feature: you cannot specify the stride in bytes. You can specify the itemSize and the offset in InterleavedBufferAttribute, and you can specify the stride in terms of number of typed-array elements.

This works fine if one point's bytesize is a multiple of your data type for one attribute, but it's not always the case. For example, if my datas are positions (32 bits) and classification (8 bits), I can specify a stride of 5 for the classification buffer, but there's no way I can specify the right stride for the positions buffer.

Do you reach the same conclusion as I did? Or did I misunderstand something? If you do I'll ask a question/open an issue in the THREE.js repository.

autra · 2018-05-29T15:30:23Z

updated (@iTowns/reviewers and @mbredif )

autra · 2018-05-29T15:30:48Z

I also pushed a proposal about the updateUniforms mechanism.

peppsac

Just left a couple comments, looks good overall.

peppsac · 2018-05-30T10:09:14Z

src/Parser/PotreeBinParser.js

+    },
+    INTENSITY: {
+        numElements: 1,
+        numByte: 2,


Maybe you should add a comment to explain why you need this? (AFAIU it's because Float16Array doesn't exist so you emulate it)

peppsac · 2018-05-30T10:11:12Z

src/Parser/PotreeBinParser.js

+        for (let pntIdx = 0; pntIdx < numPoints; pntIdx++) {
+            for (const attr of attrs) {
+                for (let elemIdx = 0; elemIdx < attr.numElements; elemIdx++) {
+                    const fnName = `getUint${(attr.numByte || attr.array.BYTES_PER_ELEMENT) * 8}`;


This could be done in the parent loop since it doesn't depend on elemIdx.

peppsac · 2018-05-30T10:15:31Z

src/Renderer/PointsMaterial.js

    }

    enablePicking(pickingMode) {
        // we don't want pixels to blend over already drawn pixels
        this.blending = pickingMode ? NoBlending : NormalBlending;
-        this.uniforms.pickingMode.value = pickingMode;
+        if (pickingMode) {
+            this.oldMode = this.uniforms.mode.value;


Shouldn't this.oldMode be also assigned in the constructor (like @elemoine mentionned here #719 (comment))?

Yes, at least initialized to an empty value, if we want to rely entirely on the update mechanism.

this code looks buggy : if you enable picking twice in a row, oldmode will be PICKING. A more sensible invariant could be "oldmode holds the last non-picking mode used" and could be initialized by the initial mode (or whatever if the initial mode is picking)

autra · 2018-06-01T09:46:32Z

@peppsac updated and rebased.

mbredif

That works great already ! here are some remarks.

Concerning interleaved buffer attributes, there seems to be issues in webgl when attributes are not type-aligned, hence the restriction. That means that creating an interleaved buffer with no attribute decoding may not always be possible. a possibility would be to have a hint option that tentatively tries it and defaults to non interleaved buffers if decoding is required.

mbredif · 2018-06-04T09:14:26Z

src/Parser/PotreeBinParser.js

+                attributeName: attr.attributeName,
+                normalized: attr.normalized,
+                array: new attr.arrayType(attr.numElements * numPoints),
+                numByte: attr.numByte,


you could apply the default here and be done with it

I don't understand what you mean by that.

here : numByte: attr.numByte || attr.array.BYTES_PER_ELEMENT,
elsewhere: attr.numByte instead of attr.numByte || attr.array.BYTES_PER_ELEMENT

mbredif · 2018-06-04T09:20:20Z

src/Parser/PotreeBinParser.js

-            positions[3 * i + 2] = view.getUint32(offset + 8, true);
+        for (let pntIdx = 0; pntIdx < numPoints; pntIdx++) {
+            for (const attr of attrs) {
+                const fnName = `getUint${(attr.numByte || attr.array.BYTES_PER_ELEMENT) * 8}`;


this could move out of the pntIdx loop, by setting it as a function of attr in its definition loop.

Maybe a comment on why getUint* works even on all types (float, Int...) (It looks strange to me)

this could move out of the pntIdx loop, by setting it as a function of attr in its definition loop.

What do we gain by that?

Maybe a comment on why getUint* works even on all types (float, Int...) (It looks strange to me)

it doesn't work on all types, it's just that potree converter stores everything as int, then putting an int into a Float32Array will convert it into a float.

What do we gain by that?

numPoints times less evaluations of view[`getUint${(attr.numByte || attr.array.BYTES_PER_ELEMENT) * 8}`] (and cleaner code IMHO :))

by setting it as a function of attr in its definition loop.

you mean as a property of attr then?

it doesn't work on all types, it's just that potree converter stores everything as int, then putting an int into a Float32Array will convert it into a float.

Indeed, by looking into it, potree is relying on a custom view object rather than a DataView. I am not sure the 2 approaches are strictly equivalent (It is not clear to me who is manipulating bytes and who is doing type casting), but if your approach works, then let us go with it !

you mean as a property of attr then?

yes

mbredif · 2018-06-04T09:21:52Z

src/Provider/PointCloudProvider.js

@@ -138,7 +139,7 @@ function loadPointFile(layer, url) {
        if (layer.metadata.customBinFormat) {
            return PotreeCinParser.parse(ab).then(result => addPickingAttribute(result));
        } else {
-            return PotreeBinParser.parse(ab).then(result => addPickingAttribute(result));
+            return PotreeBinParser.parse(ab, layer.metadata.pointAttributes).then(result => addPickingAttribute(result));


maybe the whole layer.metadata should be given for future parsers to consume it.

I'm not sure yet, that's why I did an adhoc call here. I think this question should be tackled when/if we have a parser registry mechanism for pointclouds, WDYT? Not yet sure how this will be done though (there are probably things that can be unified with 3dTiles pnts format I guess).

mbredif · 2018-06-04T09:24:53Z

src/Renderer/PointsMaterial.js

    }

    enablePicking(pickingMode) {
        // we don't want pixels to blend over already drawn pixels
        this.blending = pickingMode ? NoBlending : NormalBlending;
-        this.uniforms.pickingMode.value = pickingMode;
+        if (pickingMode) {
+            this.oldMode = this.uniforms.mode.value;


this code looks buggy : if you enable picking twice in a row, oldmode will be PICKING. A more sensible invariant could be "oldmode holds the last non-picking mode used" and could be initialized by the initial mode (or whatever if the initial mode is picking)

mbredif · 2018-06-04T09:28:10Z

src/Renderer/Shader/PointsVS.glsl

+#define MODE_CLASSIFICATION 3
+#define MODE_NORMAL 4
+#define MODE_NORMAL_OCT16 5
+#define MODE_NORMAL_SPHEREMAPPED 6


to prevent the replication of code values, material.defines could be used instead

mbredif · 2018-06-04T09:29:42Z

src/Renderer/Shader/PointsVS.glsl

+    } else if (mode == MODE_NORMAL_OCT16) {
+        vColor = vec4(decodeOct16Normal(oct16Normal), 1.0);
+    } else if (mode == MODE_NORMAL_SPHEREMAPPED) {
+        vColor = vec4(decodeSphereMappedNormal(sphereMappedNormal), 1.0);


normals range from -1 to 1. maybe you can take the absolute value of it.

autra · 2018-06-11T14:03:32Z

@iTowns/reviewers updated.

Also pinging @mbredif: I think I have adressed all your remarks.

mbredif

Apart from the inline comments, I have 2 concerns :

how well does it work with the stock THREE.PointsMaterial ? you can browse here : examples/pointcloud.html?material=three. Ensuring it works well would prevent too much coupling with itowns.PointsMaterial...
I find the oldMode logic a bit convoluted.
Maybe you could have this.picking=false and this.mode=MODE.COLOR in the constructor, remove the oldMode property and the enablePicking function (just set material.picking instead), and do this in updateUniforms :

this.blending = pickingMode ? NoBlending : NormalBlending;
 this.uniforms.mode.value = this.picking ? MODE.PICKING : this.mode;

mbredif · 2018-06-14T12:57:45Z

src/Renderer/PointsMaterial.js


        this.uniforms.size = new Uniform(this.size);
+        this.uniforms.mode = new Uniform(this.mode);
        this.uniforms.pickingMode = new Uniform(false);


unused, to be removed

Indeed, thanks for the catch :-)

mbredif · 2018-06-14T12:59:58Z

src/Provider/PointCloudProvider.js

+                    layer.metadata.pointAttributes.find(elem => elem.startsWith('NORMAL'));
+                if (normal) {
+                    layer.material.defines[normal] = 1;
+                }


what about an opt-in isNormal property for pointAttributes ? that would prevent having a dependency on this specific material...

The more general question is: do we expect users to be able subclass Material and not ShaderMaterial?

that would prevent having a dependency on this specific material

IMHO, we don't need to impose ourselves too much constraints. In the same way that TileMesh/LayeredMaterial is not replaceable by another mesh/material, we should totally rely on PointsMaterial to implement our features.

I disagree, the less coupling, the more itowns is untangled, usable and extensible... I would rather set opt-in hint properties to materials so that materials could honor the hint if possible.

On this instance, we don't really care. The only "coupling" was the fact layer.material.defines was undefined (!). The current code works with a basic three material.

I completely agree here. (We'll talk again about it when we will want to have the same functionality with the same material applied to pointclouds parsed from PNTS, PLY, LAS, LAZ...)

mbredif · 2018-06-14T13:22:19Z

src/Parser/PotreeBinParser.js

+                // Potree stores everything as int, and uses scale + offset
+                fnName: `getUint${numByte * 8}`,
+            });
+        }


Apart from array, this could be done statically once and for all in the global scope

I think I don't see what you mean, because I feel I can't. I'm constructing an array of attr that depends on pointAttributes. The point of this code is to copy the POINT_ATTTRIBUTES static array.

mbredif · 2018-06-14T13:24:22Z

src/Parser/PotreeBinParser.js

-            positions[3 * i + 2] = view.getUint32(offset + 8, true);
+        for (let pntIdx = 0; pntIdx < numPoints; pntIdx++) {
+            for (const attr of attrs) {
+                for (let elemIdx = 0; elemIdx < attr.numElements; elemIdx++) {


if the previous attr loop goes in global scope, the array could be instanced here (just after for (const attr of attrs)) and added to the geometry at the end of the iteration instead of in another loop.

Put some code please, that will be faster and clearer.

autra · 2018-06-14T16:19:06Z

how well does it work with the stock THREE.PointsMaterial ? you can browse here : examples/pointcloud.html?material=three. Ensuring it works well would prevent too much coupling with itowns.PointsMaterial...

To understand a bit more what you're after, can you tell me a bit more about your use case for other Materials?

Otherwise, please note that the debug tools won't completely work with material=three. These debug tools are all pretty much material dependant.

I find the oldMode logic a bit convoluted. Maybe you could have this.picking=false and this.mode=MODE.COLOR in the constructor, remove the oldMode property and the enablePicking function (just set material.picking instead), and do this in updateUniforms :

It's a bit convoluted, but for the sake of being clearer for the user: picking is mutually exclusive with other display modes. Having a separate boolean to set it will complicate things from a user POV.

mbredif · 2018-06-19T09:24:20Z

For the attr loop (Add support for other data types in potree pointclouds #767 (comment) and Add support for other data types in potree pointclouds #767 (comment)), see mbredif@fe850ac

The more general question is: do we expect users to be able subclass Material and not ShaderMaterial?
To understand a bit more what you're after, can you tell me a bit more about your use case for other Materials?

Ideally, I would rather not have any coupling between individual parsers, providers and materials, meaning (the includes in the potree provider would just be there to provide default parsers and materials, that could be overriden) :

we should be able to use any material, including the ones from THREE (eg THREE.PointsMaterial) or other THREE-based libraries, for most itowns objects
extra features (such as modes) should be opt-in features of the materials that need specification
parsed attributes should have semantics (isNormal, isPosition, crs...) to decouple parsers from materials : ideally parsed pointclouds would just be Features (with improvements discussed in Improving the Feature type #763).
the same itowns.PointsMaterial could be used on any pointcloud (static PLY files, Potree layers and 3Dtiles/PNTS layers...) with its improved features, but THREE.PointsMaterial would work as well.

Otherwise, please note that the debug tools won't completely work with material=three. These debug tools are all pretty much material dependant.

... unless we specify the opt-in mode feature for itowns materials. (there is already a similar concept, with a completely different implementation for the LayeredMaterial, that is weirdly managed by the TileMesh )

It's a bit convoluted, but for the sake of being clearer for the user: picking is mutually exclusive with other display modes. Having a separate boolean to set it will complicate things from a user POV.

I agree from a shader perspective that these are exclusive modes, but from a user perspective (and that is how we use it!), picking is more like an overriding mode which deserves (IMHO) an overriding property. For symmetry, I suggest to go from the boolean this.picking to a this.overrideMode property (much like THREE's overrideMaterial)

mbredif · 2018-06-19T09:27:35Z

Note that, even I had the rights, I would not block this PR, I am just expressing my opinion on the coupling it reintroduces... However that can be addressed in a follow up PR (eg: for the equivalent PR on PNTS pointclouds)

peppsac · 2018-06-29T14:53:51Z

picking is more like an overriding mode which deserves (IMHO) an overriding property. For symmetry, I suggest to go from the boolean this.picking to a this.overrideMode property

FWIW, I agree with both of you:

picking is an overriding mode so an this.overrideMode would make sense
but the only usage we have (and that I can see) of this.overrideMode is for picking, so it makes sense to use a plain boolean instead

mbredif · 2018-06-29T15:58:29Z

Now that I have reviewed most of the itowns codebase, I suggest to :

use the RenderMode enum in the core for modes of all material (possibly materials could add new render modes as well)
let materials have an opt-in material.mode property to select the mode (which could be implemented with defines, uniforms or a combination at the discretion of each material using Object.defineProperty and setting needsUpdate or uniformsNeedUpdate as required). Materials with no mode property are simply not selectable. (maybe mode support could be advertised with a function)
it is the caller's responsibility (picking, styling, depth estimation...) to backup previous state and restore it (something like pushRenderMode that would just set or restore the mode property or maybe just retraversing the tree)

autra · 2018-06-29T16:52:16Z

So I kept your first commit @mbredif (thanks for the simplification). As for the second, I suggest to open a PR with it (I'd be glad to r+ it). I have pushed it in its own branch (https://github.com/iTowns/itowns/tree/points_shader_refacto).

I reintroduced a picking boolean as you and @peppsac suggested, and restored the compatibility with THREE material (I even adapted PointCloudDebug to support it by making picking and mode opt-in).

@iTowns/reviewers please r? :-)

peppsac

Looks good (I left a suggestion for the shader code)

peppsac · 2018-07-03T07:12:55Z

src/Provider/PointCloudProvider.js

+                    layer.metadata.pointAttributes.find(elem => elem.startsWith('NORMAL'));
+                if (normal) {
+                    layer.material.defines[normal] = 1;
+                }


that would prevent having a dependency on this specific material

IMHO, we don't need to impose ourselves too much constraints. In the same way that TileMesh/LayeredMaterial is not replaceable by another mesh/material, we should totally rely on PointsMaterial to implement our features.

peppsac · 2018-07-03T07:20:15Z

src/Renderer/Shader/PointsVS.glsl

+    } else if (mode == MODE_INTENSITY) {
+        vColor = vec4(intensity, intensity, intensity, opacity);
+    } else if (mode == MODE_NORMAL) {
+#if defined(NORMAL_OCT16)


I'd like to separate the normal parsing from colorisation mode.
Something like:

#if defined(NORMAL_OCT16) vec3 normal = decodeOct16Normal(...); #elif defined(NORMAL_SPHEREMAPPED) vec3 normal = decodeSphereMappedNormal(sphereMappedNormal); #elif defined(NORMAL) // nothing to do #else vec3 normal; #endif if (pickingMode) { vColor = unique_id; vColor = unique_id; } else if (mode == MODE_INTENSITY) { vColor = vec4(intensity, intensity, intensity, opacity); } else if (mode == MODE_NORMAL) { vColor = vec4(abs(normal), opacity); } else { ... }

I agree, I wrote a commit about it with shader chunks : 58c6c21

peppsac · 2018-07-03T07:20:36Z

test/PotreeBinParser_unit_test.js

+const assert = require('assert');
+
+
+describe('PotreeBinParser', function () {


gchoqueux

we should add tests on :
PointCloudProvider.js
PointsMaterial.js

peppsac · 2018-07-03T09:04:08Z

gchoqueux requested changes
we should add tests on :
PointCloudProvider.js
PointsMaterial.js

😫

@gchoqueux you should take a step back and think if you're attitude is really helping the itowns project (hint: it is not).

autra · 2018-07-03T09:31:45Z

@gchoqueux what test I can add for these 2 modules is unclear to me. Can you push a test example in a branch to see what you have in mind?

autra · 2018-07-09T12:25:01Z

@gchoqueux I pushed a test for PointsCloudProvider.js. On the other hand, I don't think PointsMaterial really needs any testing.

autra · 2018-07-09T13:05:42Z

@iTowns/reviewers please r? again :-)

zarov

Seems good to me, I don't see either which test could be added to PointsMaterial.

zarov · 2018-07-10T09:40:04Z

src/Provider/PointCloudProvider.js

@@ -272,3 +274,7 @@ export default {
        });
    },
 };
+
+export const _testing = {
+    parseMetadata,


I'm not okay with this, please move it to the default export, even if I understand that this is for testing purpose only.

Thanks for your review. Can you elaborate on the reasons you don't like this?

To elaborate on my position, I'm pretty against exposing this to the default export, because it's a private method that has no reason to be exposed. Once you expose such a method, it becomes part of the public api, which means we have an implicit commitment to it. I know that this way of thinking is not very common on this project, but I'd like not to introduce api breakage when we can avoid it. For me _testing is a good middle-ground as it conveys the fact it is a private method.

I don't really know, maybe in fact it is a good idea. We could agree on a special exported namespace we could use to test internal methods. So in fact maybe keep it and find a more suitable name ? _testing feels muddled to me.

Once you expose such a method, it becomes part of the public api

Providers are not in the public API so no problems here, but see my change of pov above ;)

Providers are not in the public API so no problems here

Just a quick note on this: I'm afraid they are. Everything importable from a ES6 configuration is part of the public API and we should treat it that way. Every transpiled file in lib is importable. Actually, in my opinion this should be the preferred way of using iTowns. The 2 Main files are just convenience.

So in fact maybe keep it and find a more suitable name ? _testing feels muddled to me.

I don't know, testing seemed a suitable name for an export for testing purpose... I don't see a better name (yet).

Ok I see one possible solution: surround testing exports with __DEBUG__ tests. But I need to add support for this in our .babelrc. So as not to enter into a rabbit hole for this PR, I suggest to keep it that way here and revisit it later. Is it acceptable for you @zarov?

(and then rename __DEBUG__ with __DEV__)

(sorry for being so undecided) yeah testing is good :D But if you feel that surrounding testing with those is better, let's go for it.

autra · 2018-07-10T09:56:53Z

Btw, I'm not quite happy with the name and the return value of parseMetadata (they don't really match). Do you guys have a better suggestion?

mbredif · 2018-07-10T10:22:33Z

as parseMetadata and parseOctree are parse commands, should not they be part of a 'parsers/PotreeCloudParser.js' file (or something similar) ?

autra · 2018-07-10T11:32:36Z

as parseMetadata and parseOctree are parse commands, should not they be part of a 'parsers/PotreeCloudParser.js' file (or something similar) ?

Not sure. We use parse here, but it's not really the case, we are just reading protocol-specific metadata (so their place is in a Provider). Maybe they should be renamed though. I'm not happy with this parseMetadata name anyway.

autra · 2018-07-10T14:43:42Z

Ok, I pushed another version with parseMetadata renamed into setLayerMetadata. @peppsac @zarov WDYT?

autra · 2018-07-10T14:56:20Z

actually, setXXX should not be that complex (should be a one liner), reverting to parseXXXX. I'm sure that this won't threaten the future of iTowns to have a parseXXX (private) method in a Provider ;-)

Moving forward

autra commented May 28, 2018

View reviewed changes

mbredif reviewed May 28, 2018

View reviewed changes

autra force-pushed the potree_attributes branch from 3c4877b to ff6e1ed Compare May 29, 2018 15:29

peppsac reviewed May 30, 2018

View reviewed changes

mbredif mentioned this pull request May 31, 2018

refactor(parser) : move Materials from potree parsers to potree provider (+custom layer material option) #764

Merged

autra force-pushed the potree_attributes branch from ff6e1ed to 29fa625 Compare June 1, 2018 09:30

mbredif reviewed Jun 4, 2018

View reviewed changes

gchoqueux mentioned this pull request Jun 5, 2018

refacto(core): add or use optional target to getPickingPositionFromDepth #772

Merged

autra force-pushed the potree_attributes branch 2 times, most recently from 5d387f0 to 23da8d3 Compare June 11, 2018 14:01

mbredif reviewed Jun 14, 2018

View reviewed changes

mbredif force-pushed the potree_attributes branch from ad0c4e3 to 58c6c21 Compare June 19, 2018 10:12

autra force-pushed the potree_attributes branch 2 times, most recently from 005bb62 to 371ae5a Compare June 29, 2018 14:27

peppsac approved these changes Jul 3, 2018

View reviewed changes

gchoqueux previously requested changes Jul 3, 2018

View reviewed changes

autra force-pushed the potree_attributes branch from ea780e8 to 3e36b98 Compare July 9, 2018 12:41

autra and others added 6 commits July 10, 2018 11:17

core(PointCloud): add support for other data type in potree pointclouds

a250f90

refactor(parsers) attr loop optimization in PotreeBinParser

b319777

review

79b08d8

Add unit test

e2f7e92

fix linter

276c504

Separate normal decoding from vColor calculation

d2e1f55

autra force-pushed the potree_attributes branch from ee99279 to d2e1f55 Compare July 10, 2018 09:17

zarov reviewed Jul 10, 2018

View reviewed changes

Change fn name

93b3e27

revert to parseMetadata

bb7e431

autra merged commit d21bd10 into master Jul 12, 2018

gchoqueux deleted the potree_attributes branch January 22, 2019 12:16

		const assert = require('assert');


		describe('PotreeBinParser', function () {

Add support for other data types in potree pointclouds #767

Add support for other data types in potree pointclouds #767

Conversation

autra commented May 28, 2018 • edited Loading

Description

Choose a reason for hiding this comment

mbredif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbredif May 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbredif commented May 28, 2018

autra commented May 29, 2018

autra commented May 29, 2018

autra commented May 29, 2018

autra commented May 29, 2018

peppsac 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

autra commented Jun 1, 2018

mbredif 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

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

mbredif Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

autra commented Jun 11, 2018

mbredif 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

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

autra commented Jun 14, 2018

mbredif commented Jun 19, 2018 • edited Loading

mbredif commented Jun 19, 2018 • edited Loading

peppsac commented Jun 29, 2018

mbredif commented Jun 29, 2018

autra commented Jun 29, 2018

peppsac 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

gchoqueux left a comment • edited Loading

Choose a reason for hiding this comment

peppsac commented Jul 3, 2018 • edited Loading

autra commented Jul 3, 2018

autra commented Jul 9, 2018

autra commented Jul 9, 2018

zarov left a comment

Choose a reason for hiding this comment

autra commented May 28, 2018 •

edited

Loading

mbredif May 28, 2018 •

edited

Loading

mbredif Jun 4, 2018 •

edited

Loading

mbredif commented Jun 19, 2018 •

edited

Loading

mbredif commented Jun 19, 2018 •

edited

Loading

gchoqueux left a comment •

edited

Loading

peppsac commented Jul 3, 2018 •

edited

Loading

mbredif commented Jul 10, 2018 •

edited

Loading