Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for other data types in potree pointclouds #767

Merged
merged 8 commits into from
Jul 12, 2018
Merged

Add support for other data types in potree pointclouds #767

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a250f90
core(PointCloud): add support for other data type in potree pointclouds
autra May 28, 2018
b319777
refactor(parsers) attr loop optimization in PotreeBinParser
mbredif Jun 19, 2018
79b08d8
review
autra Jun 29, 2018
e2f7e92
Add unit test
autra Jul 9, 2018
276c504
fix linter
autra Jul 9, 2018
d2e1f55
Separate normal decoding from vColor calculation
autra Jul 9, 2018
93b3e27
Change fn name
autra Jul 10, 2018
bb7e431
revert to parseMetadata
autra Jul 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 85 additions & 30 deletions src/Parser/PotreeBinParser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,107 @@
import * as THREE from 'three';

// See the different constants holding ordinal, name, numElements, byteSize in PointAttributes.cpp in PotreeConverter
// elementByteSize is byteSize / numElements
const POINT_ATTTRIBUTES = {
POSITION_CARTESIAN: {
numElements: 3,
arrayType: Float32Array,
attributeName: 'position',
},
COLOR_PACKED: {
numElements: 4,
arrayType: Uint8Array,
attributeName: 'color',
normalized: true,
},
INTENSITY: {
numElements: 1,
numByte: 2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// using Float32Array because Float16Array doesn't exist
arrayType: Float32Array,
attributeName: 'intensity',
normalized: true,
},
CLASSIFICATION: {
numElements: 1,
arrayType: Uint8Array,
attributeName: 'classification',
},
// Note: at the time of writing, PotreeConverter will only generate normals in Oct16 format
// see PotreeConverter.cpp:121
// we keep all the historical value to still supports old conversion
NORMAL_SPHEREMAPPED: {
numElements: 2,
arrayType: Uint8Array,
attributeName: 'sphereMappedNormal',
},
// see https://web.archive.org/web/20150303053317/http://lgdv.cs.fau.de/get/1602
NORMAL_OCT16: {
numElements: 2,
arrayType: Uint8Array,
attributeName: 'oct16Normal',
},
NORMAL: {
numElements: 3,
arrayType: Float32Array,
attributeName: 'normal',
},
};

for (const potreeName of Object.keys(POINT_ATTTRIBUTES)) {
const attr = POINT_ATTTRIBUTES[potreeName];
attr.potreeName = potreeName;
attr.numByte = attr.numByte || attr.arrayType.BYTES_PER_ELEMENT;
attr.byteSize = attr.numElements * attr.numByte;
attr.normalized = attr.normalized || false;
// chrome is known to perform badly when we call a method without respecting its arity
const fnName = `getUint${attr.numByte * 8}`;
attr.getValue = attr.numByte === 1 ?
function getValue(view, offset) { return view[fnName](offset); } :
function getValue(view, offset) { return view[fnName](offset, true); };
}

export default {
/** @module PotreeBinParser */
/** Parse .bin PotreeConverter format and convert to a THREE.BufferGeometry
* @function parse
* @param {ArrayBuffer} buffer - the bin buffer.
* @param {Object} pointAttributes - the point attributes information contained in layer.metadata coming from cloud.js
* @return {Promise} - a promise that resolves with a THREE.BufferGeometry.
*
*/
parse: function parse(buffer) {
parse: function parse(buffer, pointAttributes) {
if (!buffer) {
throw new Error('No array buffer provided.');
}

const view = new DataView(buffer);
// Format: X1,Y1,Z1,R1,G1,B1,A1,[...],XN,YN,ZN,RN,GN,BN,AN
const numPoints = Math.floor(buffer.byteLength / 16);

const positions = new Float32Array(3 * numPoints);
const colors = new Uint8Array(4 * numPoints);

const box = new THREE.Box3();
box.min.set(Infinity, Infinity, Infinity);
box.max.set(-Infinity, -Infinity, -Infinity);
const tmp = new THREE.Vector3();

let offset = 0;
for (let i = 0; i < numPoints; i++) {
positions[3 * i] = view.getUint32(offset + 0, true);
positions[3 * i + 1] = view.getUint32(offset + 4, true);
positions[3 * i + 2] = view.getUint32(offset + 8, true);

tmp.fromArray(positions, 3 * i);
box.min.min(tmp);
box.max.max(tmp);

colors[4 * i] = view.getUint8(offset + 12);
colors[4 * i + 1] = view.getUint8(offset + 13);
colors[4 * i + 2] = view.getUint8(offset + 14);
colors[4 * i + 3] = 255;

offset += 16;
let pointByteSize = 0;
for (const potreeName of pointAttributes) {
pointByteSize += POINT_ATTTRIBUTES[potreeName].byteSize;
}
const numPoints = Math.floor(buffer.byteLength / pointByteSize);

const geometry = new THREE.BufferGeometry();
geometry.addAttribute('position', new THREE.BufferAttribute(positions, 3));
geometry.addAttribute('color', new THREE.BufferAttribute(colors, 4, true));
geometry.boundingBox = box;
let elemOffset = 0;
let attrOffset = 0;
for (const potreeName of pointAttributes) {
const attr = POINT_ATTTRIBUTES[potreeName];
const arrayLength = attr.numElements * numPoints;
const array = new attr.arrayType(arrayLength);
for (let arrayOffset = 0; arrayOffset < arrayLength; arrayOffset += attr.numElements) {
for (let elemIdx = 0; elemIdx < attr.numElements; elemIdx++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

array[arrayOffset + elemIdx] = attr.getValue(view, attrOffset + elemIdx * attr.numByte);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

attrOffset += pointByteSize;
}
elemOffset += attr.byteSize;
attrOffset = elemOffset;
geometry.addAttribute(attr.attributeName, new THREE.BufferAttribute(array, attr.numElements, attr.normalized));
}

geometry.computeBoundingBox();

return Promise.resolve(geometry);
},
Expand Down
123 changes: 69 additions & 54 deletions src/Provider/PointCloudProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import Fetcher from './Fetcher';
import PointCloudProcessing from '../Process/PointCloudProcessing';
import PotreeBinParser from '../Parser/PotreeBinParser';
import PotreeCinParser from '../Parser/PotreeCinParser';
import PointsMaterial from '../Renderer/PointsMaterial';
import PointsMaterial, { MODE } from '../Renderer/PointsMaterial';
import Picking from '../Core/Picking';
import Extent from '../Core/Geographic/Extent';

Expand Down Expand Up @@ -142,6 +142,55 @@ function addPickingAttribute(points) {
return points;
}

function parseMetadata(layer, metadata) {
layer.metadata = metadata;

let bbox;
var customBinFormat = true;

// Lopocs pointcloud server can expose the same file structure as PotreeConverter output.
// The only difference is the metadata root file (cloud.js vs infos/sources), and we can
// check for the existence of a `scale` field.
// (if `scale` is defined => we're fetching files from PotreeConverter)
if (layer.metadata.scale != undefined) {
// PotreeConverter format
customBinFormat = layer.metadata.pointAttributes === 'CIN';
bbox = new THREE.Box3(
new THREE.Vector3(metadata.boundingBox.lx, metadata.boundingBox.ly, metadata.boundingBox.lz),
new THREE.Vector3(metadata.boundingBox.ux, metadata.boundingBox.uy, metadata.boundingBox.uz));

// do we have normal information
const normal = Array.isArray(layer.metadata.pointAttributes) &&
layer.metadata.pointAttributes.find(elem => elem.startsWith('NORMAL'));
if (normal) {
layer.material.defines[normal] = 1;
}
} else {
// Lopocs
layer.metadata.scale = 1;
layer.metadata.octreeDir = `itowns/${layer.table}.points`;
layer.metadata.hierarchyStepSize = 1000000; // ignore this with lopocs
customBinFormat = true;

let idx = 0;
for (const entry of metadata) {
if (entry.table == layer.table) {
break;
}
idx++;
}
bbox = new THREE.Box3(
new THREE.Vector3(metadata[idx].bbox.xmin, metadata[idx].bbox.ymin, metadata[idx].bbox.zmin),
new THREE.Vector3(metadata[idx].bbox.xmax, metadata[idx].bbox.ymax, metadata[idx].bbox.zmax));
}

layer.parse = customBinFormat ? PotreeCinParser.parse : PotreeBinParser.parse;
layer.extension = customBinFormat ? 'cin' : 'bin';
layer.supportsProgressiveDisplay = customBinFormat;

return bbox;
}

export default {
preprocessDataLayer(layer, view) {
if (!layer.file) {
Expand Down Expand Up @@ -169,6 +218,8 @@ export default {
layer.type = 'geometry';
layer.material = layer.material || {};
layer.material = layer.material.isMaterial ? layer.material : new PointsMaterial(layer.material);
layer.material.defines = layer.material.defines || {};
layer.mode = MODE.COLOR;

// default update methods
layer.preUpdate = PointCloudProcessing.preUpdate;
Expand All @@ -178,58 +229,18 @@ export default {
// this probably needs to be moved to somewhere else
layer.pickObjectsAt = (view, mouse, radius) => Picking.pickPointsAt(view, mouse, radius, layer);

return Fetcher.json(`${layer.url}/${layer.file}`, layer.fetchOptions).then((cloud) => {
layer.metadata = cloud;

let bbox;
var customBinFormat = true;

// Lopocs pointcloud server can expose the same file structure as PotreeConverter output.
// The only difference is the metadata root file (cloud.js vs infos/sources), and we can
// check for the existence of a `scale` field.
// (if `scale` is defined => we're fetching files from PotreeConverter)
if (layer.metadata.scale != undefined) {
// PotreeConverter format
customBinFormat = layer.metadata.pointAttributes === 'CIN';
bbox = new THREE.Box3(
new THREE.Vector3(cloud.boundingBox.lx, cloud.boundingBox.ly, cloud.boundingBox.lz),
new THREE.Vector3(cloud.boundingBox.ux, cloud.boundingBox.uy, cloud.boundingBox.uz));
} else {
// Lopocs
layer.metadata.scale = 1;
layer.metadata.octreeDir = `itowns/${layer.table}.points`;
layer.metadata.hierarchyStepSize = 1000000; // ignore this with lopocs
customBinFormat = true;

let idx = 0;
for (const entry of cloud) {
if (entry.table == layer.table) {
break;
}
idx++;
}
bbox = new THREE.Box3(
new THREE.Vector3(cloud[idx].bbox.xmin, cloud[idx].bbox.ymin, cloud[idx].bbox.zmin),
new THREE.Vector3(cloud[idx].bbox.xmax, cloud[idx].bbox.ymax, cloud[idx].bbox.zmax));
}

layer.parse = customBinFormat ? PotreeCinParser.parse : PotreeBinParser.parse;
layer.extension = customBinFormat ? 'cin' : 'bin';
layer.supportsProgressiveDisplay = customBinFormat;

return parseOctree(
layer,
layer.metadata.hierarchyStepSize,
{ baseurl: `${layer.url}/${cloud.octreeDir}/r`, name: '', bbox });
}).then((root) => {
// eslint-disable-next-line no-console
console.log('LAYER metadata:', root);
layer.root = root;
root.findChildrenByName = findChildrenByName.bind(root, root);
layer.extent = Extent.fromBox3(view.referenceCrs, root.bbox);

return layer;
});
return Fetcher.json(`${layer.url}/${layer.file}`, layer.fetchOptions)
.then(metadata => parseMetadata(layer, metadata))
.then(bbox => parseOctree(layer, layer.metadata.hierarchyStepSize, { baseurl: `${layer.url}/${layer.metadata.octreeDir}/r`, name: '', bbox }))
.then((root) => {
// eslint-disable-next-line no-console
console.log('LAYER metadata:', root);
layer.root = root;
root.findChildrenByName = findChildrenByName.bind(root, root);
layer.extent = Extent.fromBox3(view.referenceCrs, root.bbox);

return layer;
});
},

executeCommand(command) {
Expand All @@ -247,7 +258,7 @@ export default {
// when we request .hrc files)
const url = `${node.baseurl}/r${node.name}.${layer.extension}?isleaf=${command.isLeaf ? 1 : 0}`;

return Fetcher.arrayBuffer(url, layer.fetchOptions).then(layer.parse).then((geometry) => {
return Fetcher.arrayBuffer(url, layer.fetchOptions).then(buffer => layer.parse(buffer, layer.metadata.pointAttributes)).then((geometry) => {
const points = new THREE.Points(geometry, layer.material.clone());
addPickingAttribute(points);
points.frustumCulled = false;
Expand All @@ -263,3 +274,7 @@ export default {
});
},
};

export const _testing = {
parseMetadata,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and then rename __DEBUG__ with __DEV__)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

};
37 changes: 28 additions & 9 deletions src/Renderer/PointsMaterial.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@ import PointsVS from './Shader/PointsVS.glsl';
import PointsFS from './Shader/PointsFS.glsl';
import Capabilities from '../Core/System/Capabilities';

export const MODE = {
COLOR: 0,
INTENSITY: 1,
CLASSIFICATION: 2,
NORMAL: 3,
};

class PointsMaterial extends RawShaderMaterial {
constructor(options = {}) {
super(options);
Expand All @@ -12,17 +19,24 @@ class PointsMaterial extends RawShaderMaterial {
this.size = options.size || 0;
this.scale = options.scale || 0.05 * 0.5 / Math.tan(1.0 / 2.0); // autosizing scale
this.overlayColor = options.overlayColor || new Vector4(0, 0, 0, 0);
this.mode = options.mode || MODE.COLOR;
this.picking = false;

for (const key in MODE) {
if (Object.prototype.hasOwnProperty.call(MODE, key)) {
this.defines[`MODE_${key}`] = MODE[key];
}
}

this.uniforms.size = new Uniform(this.size);
this.uniforms.pickingMode = new Uniform(false);
this.uniforms.mode = new Uniform(this.mode);
this.uniforms.pickingMode = new Uniform(this.picking);
this.uniforms.opacity = new Uniform(this.opacity);
this.uniforms.overlayColor = new Uniform(this.overlayColor);

if (Capabilities.isLogDepthBufferSupported()) {
this.defines = {
USE_LOGDEPTHBUF: 1,
USE_LOGDEPTHBUF_EXT: 1,
};
this.defines.USE_LOGDEPTHBUF = 1;
this.defines.USE_LOGDEPTHBUF_EXT = 1;
}

if (__DEBUG__) {
Expand All @@ -31,15 +45,17 @@ class PointsMaterial extends RawShaderMaterial {
this.updateUniforms();
}

enablePicking(pickingMode) {
// we don't want pixels to blend over already drawn pixels
this.uniforms.pickingMode.value = pickingMode;
this.blending = pickingMode ? NoBlending : NormalBlending;
enablePicking(picking) {
this.picking = picking;
this.blending = picking ? NoBlending : NormalBlending;
this.updateUniforms();
}

updateUniforms() {
// if size is null, switch to autosizing using the canvas height
this.uniforms.size.value = (this.size > 0) ? this.size : -this.scale * window.innerHeight;
this.uniforms.mode.value = this.mode;
this.uniforms.pickingMode.value = this.picking;
this.uniforms.opacity.value = this.opacity;
this.uniforms.overlayColor.value = this.overlayColor;
}
Expand All @@ -49,9 +65,12 @@ class PointsMaterial extends RawShaderMaterial {
this.opacity = source.opacity;
this.transparent = source.transparent;
this.size = source.size;
this.mode = source.mode;
this.picking = source.picking;
this.scale = source.scale;
this.overlayColor.copy(source.overlayColor);
this.updateUniforms();
Object.assign(this.defines, source.defines);
return this;
}
}
Expand Down
Loading