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

feat(core): allow layer to specify which elements should be updatable #789

Merged
merged 1 commit into from
Jul 12, 2018
Merged

feat(core): allow layer to specify which elements should be updatable #789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions src/Core/Layer/Layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,21 @@ function GeometryLayer(id, object3d) {
// Setup default picking method
this.pickObjectsAt = (view, mouse, radius) => Picking.pickObjectsAt(view, mouse, radius, this.object3d);

// Attached layers expect to receive the visual representation of a layer (= THREE object with a material).
// So if a layer's update function don't process this kind of object, the layer must provide
// a getObjectToUpdateForAttachedLayers function that returns the correct object to update for attached
// layer.
// See 3dtilesProvider or PointCloudProvider for examples.
// eslint-disable-next-line arrow-body-style
this.getObjectToUpdateForAttachedLayers = (obj) => {
if (obj.parent && obj.material) {
return {
element: obj,
parent: obj.parent,
};
}
};

this.postUpdate = () => {};
}

Expand Down
35 changes: 31 additions & 4 deletions src/Core/MainLoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,37 @@ function updateElements(context, geometryLayer, elements) {
// and then update Debug.js:addGeometryLayerDebugFeatures
const newElementsToUpdate = geometryLayer.update(context, geometryLayer, element);

// update attached layers
for (const attachedLayer of geometryLayer._attachedLayers) {
if (attachedLayer.ready) {
attachedLayer.update(context, attachedLayer, element);
const sub = geometryLayer.getObjectToUpdateForAttachedLayers(element);

if (sub) {
if (sub.element) {
if (__DEBUG__) {
if (!(sub.element.isObject3D)) {
throw new Error(`
Invalid object for attached layer to update.
Must be a THREE.Object and have a THREE.Material`);
}
}
// update attached layers
for (const attachedLayer of geometryLayer._attachedLayers) {
if (attachedLayer.ready) {
attachedLayer.update(context, attachedLayer, sub.element, sub.parent);
}
}
} else if (sub.elements) {
Copy link
Contributor

Choose a reason for hiding this comment

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

getObjectToUpdateForAttachedLayers can always return an Array?
it would simplify this part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is internal only (= no user of iTowns will have to write the same code in its application), so I don't mind having a bit more complicated code if it performs better.

I'll check, and simplify if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that it is more of a problem of structure of our elements. We should not complicate this part of the process to be able to adapt to all structures. I'll go deeper and I'll come back on the subject

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @gchoqueux, even if it is hidden, it should always return elements as an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep in mind that this piece of code is executed multiple times during each update loop.

So creating an array by default, even if it's for a single element might pressure the GC or cause a perf hit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that's a good point. We still have duplicated code though, could you move both part into another function ?

for (let i = 0; i < sub.elements.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use for (let element of sub.elements) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dislike more and more the for of syntax because it's harder to debug (lots of indirection). I'm also wondering on the perf cost of this (for the same reason: lots of indirection)...

Anyway I'll change it because for now we settled on using it everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it seems that performance wise it is the slowest (as tested quickly here http://jsben.ch/iEYTg)

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 this test misses one of the cost: in itowns we never run native for of loops, but only transpiled for of.
For instance a simple:

for (const word of words) {
  console.log(word);
}

becomes:

"use strict";

var _iteratorNormalCompletion = true;
var _didIteratorError = false;
var _iteratorError = undefined;

try {
  for (var _iterator = words[Symbol.iterator](), _step; !(_iteratorNormalCompletion = (_step = _iterator.next()).done); _iteratorNormalCompletion = true) {
    var word = _step.value;

    console.log(word);
  }
} catch (err) {
  _didIteratorError = true;
  _iteratorError = err;
} finally {
  try {
    if (!_iteratorNormalCompletion && _iterator.return) {
      _iterator.return();
    }
  } finally {
    if (_didIteratorError) {
      throw _iteratorError;
    }
  }
}

(see it live)

if (!(sub.elements[i].isObject3D)) {
throw new Error(`
Invalid object for attached layer to update.
Must be a THREE.Object and have a THREE.Material`);
}
// update attached layers
for (const attachedLayer of geometryLayer._attachedLayers) {
if (attachedLayer.ready) {
attachedLayer.update(context, attachedLayer, sub.elements[i], sub.parent);
}
}
}
}
}
updateElements(context, geometryLayer, newElementsToUpdate);
Expand Down
17 changes: 10 additions & 7 deletions src/Process/3dTilesProcessing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ function requestNewTile(view, scheduler, geometryLayer, metadata, parent, redraw
return scheduler.execute(command);
}

function getChildTiles(tile) {
// only keep children that have the same layer and a valid tileId
return tile.children.filter(n => n.layer == tile.layer && n.tileId);
}

function subdivideNode(context, layer, node, cullingTest) {
if (node.additiveRefinement) {
// Additive refinement can only fetch visible children.
Expand Down Expand Up @@ -88,7 +93,7 @@ function _subdivideNodeAdditive(context, layer, node, cullingTest) {
}

function _subdivideNodeSubstractive(context, layer, node) {
if (!node.pendingSubdivision && node.children.filter(n => n.layer == layer).length == 0) {
if (!node.pendingSubdivision && getChildTiles(node).length == 0) {
const childrenTiles = layer.tileIndex.index[node.tileId].children;
if (childrenTiles === undefined || childrenTiles.length === 0) {
return;
Expand Down Expand Up @@ -195,7 +200,7 @@ function cleanup3dTileset(layer, n, depth = 0) {
n.parent.remove(n);
}
} else {
const tiles = n.children.filter(n => n.tileId != undefined);
const tiles = getChildTiles(n);
n.remove(...tiles);
}
}
Expand Down Expand Up @@ -330,19 +335,19 @@ export function process3dTilesNode(cullingTest, subdivisionTest) {
subdivideNode(context, layer, node, cullingTest);
// display iff children aren't ready
setDisplayed(node, node.pendingSubdivision || node.additiveRefinement);
returnValue = node.children.filter(n => n.layer == layer);
returnValue = getChildTiles(node);
} else {
setDisplayed(node, true);

for (const n of node.children.filter(n => n.layer == layer)) {
for (const n of getChildTiles(node)) {
n.visible = false;
markForDeletion(layer, n);
}
}
// toggle wireframe
if (node.content && node.content.visible) {
node.content.traverse((o) => {
if (o.material) {
if (o.layer == layer && o.material) {
o.material.wireframe = layer.wireframe;
}
});
Expand All @@ -351,8 +356,6 @@ export function process3dTilesNode(cullingTest, subdivisionTest) {
}

markForDeletion(layer, node);

return undefined;
};
}

Expand Down
25 changes: 9 additions & 16 deletions src/Process/LayeredMaterialNodeProcessing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,7 @@ function checkNodeElevationTextureValidity(texture, noDataValue) {
tData[l - Math.sqrt(l)] > noDataValue;
}

export function updateLayeredMaterialNodeImagery(context, layer, node) {
if (!node.parent) {
return;
}

export function updateLayeredMaterialNodeImagery(context, layer, node, parent) {
const material = node.material;

// Initialisation
Expand All @@ -156,10 +152,10 @@ export function updateLayeredMaterialNodeImagery(context, layer, node) {
// because even if this tile is outside of the layer, it could inherit it's
// parent texture
if (!layer.noTextureParentOutsideLimit &&
node.parent &&
node.parent.material &&
node.parent.getIndexLayerColor &&
node.parent.getIndexLayerColor(layer.id) >= 0) {
parent &&
parent.material &&
parent.getIndexLayerColor &&
parent.getIndexLayerColor(layer.id) >= 0) {
// ok, we're going to inherit our parent's texture
} else {
node.layerUpdateState[layer.id].noMoreUpdatePossible();
Expand All @@ -185,7 +181,7 @@ export function updateLayeredMaterialNodeImagery(context, layer, node) {
const sequence = ImageryLayers.getColorLayersIdOrderedBySequence(imageryLayers);
material.setSequence(sequence);

initNodeImageryTexturesFromParent(node, node.parent, layer);
initNodeImageryTexturesFromParent(node, parent, layer);
}

// Proposed new process, two separate processes:
Expand Down Expand Up @@ -303,10 +299,7 @@ export function updateLayeredMaterialNodeImagery(context, layer, node) {
});
}

export function updateLayeredMaterialNodeElevation(context, layer, node) {
if (!node.parent) {
return;
}
export function updateLayeredMaterialNodeElevation(context, layer, node, parent) {
// TODO: we need either
// - compound or exclusive layers
// - support for multiple elevation layers
Expand All @@ -320,7 +313,7 @@ export function updateLayeredMaterialNodeElevation(context, layer, node) {
// Init elevation layer, and inherit from parent if possible
if (node.layerUpdateState[layer.id] === undefined) {
node.layerUpdateState[layer.id] = new LayerUpdateState();
initNodeElevationTextureFromParent(node, node.parent, layer);
initNodeElevationTextureFromParent(node, parent, layer);
currentElevation = material.getElevationLayerLevel();
const minLevel = layer.options.zoom ? layer.options.zoom.min : 0;
if (currentElevation >= minLevel) {
Expand Down Expand Up @@ -401,7 +394,7 @@ export function updateLayeredMaterialNodeElevation(context, layer, node) {
// Quick check to avoid using elevation texture with no data value
// If we have no data values, we use value from the parent tile
// We should later implement multi elevation layer to choose the one to use at each level
insertSignificantValuesFromParent(terrain.texture, node, node.parent, layer);
insertSignificantValuesFromParent(terrain.texture, node, parent, layer);
}

node.setTextureElevation(terrain);
Expand Down
7 changes: 3 additions & 4 deletions src/Process/PointCloudProcessing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ export default {
layer.group.add(elt.obj);
elt.obj.updateMatrixWorld(true);

elt.obj.owner = elt;
elt.promise = null;
}, (err) => {
if (err instanceof CancelledCommandException) {
Expand Down Expand Up @@ -265,7 +264,7 @@ export default {
// This format doesn't require points to be evenly distributed, so
// we're going to sort the nodes by "importance" (= on screen size)
// and display only the first N nodes
layer.group.children.sort((p1, p2) => p2.owner.sse - p1.owner.sse);
layer.group.children.sort((p1, p2) => p2.userData.metadata.sse - p1.userData.metadata.sse);

let limitHit = false;
layer.displayedCount = 0;
Expand All @@ -284,15 +283,15 @@ export default {
const now = Date.now();
for (let i = layer.group.children.length - 1; i >= 0; i--) {
const obj = layer.group.children[i];
if (!obj.material.visible && (now - obj.owner.notVisibleSince) > 10000) {
if (!obj.material.visible && (now - obj.userData.metadata.notVisibleSince) > 10000) {
// remove from group
layer.group.children.splice(i, 1);

obj.material.dispose();
obj.geometry.dispose();
obj.material = null;
obj.geometry = null;
obj.owner.obj = null;
obj.userData.metadata.obj = null;

if (__DEBUG__) {
if (obj.boxHelper) {
Expand Down
28 changes: 27 additions & 1 deletion src/Provider/3dTilesProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { init3dTilesLayer } from '../Process/3dTilesProcessing';
import utf8Decoder from '../utils/Utf8Decoder';

export function $3dTilesIndex(tileset, baseURL) {
let counter = 0;
let counter = 1;
this.index = {};
const inverseTileTransform = new THREE.Matrix4();
const recurse = function recurse_f(node, baseURL, parent) {
Expand Down Expand Up @@ -60,9 +60,33 @@ export function $3dTilesIndex(tileset, baseURL) {
};
}

export function getObjectToUpdateForAttachedLayers(meta) {
if (meta.content) {
const result = [];
meta.content.traverse((obj) => {
if (obj.isObject3D && obj.material && obj.layer == meta.layer) {
result.push(obj);
}
});
const p = meta.parent;
if (p && p.content) {
return {
elements: result,
parent: p.content,
};
} else {
return {
elements: result,
};
}
}
}

function preprocessDataLayer(layer, view, scheduler) {
layer.sseThreshold = layer.sseThreshold || 16;
layer.cleanupDelay = layer.cleanupDelay || 1000;
// override the default method, since updated objects are metadata in this case
layer.getObjectToUpdateForAttachedLayers = getObjectToUpdateForAttachedLayers;

layer._cleanableTiles = [];
return Fetcher.json(layer.url, layer.networkOptions).then((tileset) => {
Expand Down Expand Up @@ -180,6 +204,8 @@ function executeCommand(command) {

const setLayer = (obj) => {
obj.layers.set(layer.threejsLayer);
obj.userData.metadata = metadata;
obj.layer = layer;
};
if (path) {
// Check if we have relative or absolute url (with tileset's lopocs for example)
Expand Down
33 changes: 27 additions & 6 deletions src/Provider/PointCloudProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ function parseOctree(layer, hierarchyStepSize, root) {
baseurl: url,
bbox: bounds,
layer,
parent: snode,
};
snode.children.push(item);
stack.push(item);
Expand Down Expand Up @@ -196,6 +197,22 @@ function parseMetadata(metadata, layer) {
layer.supportsProgressiveDisplay = customBinFormat;
}

export function getObjectToUpdateForAttachedLayers(meta) {
if (meta.obj) {
const p = meta.parent;
if (p && p.obj) {
return {
element: meta.obj,
parent: p.obj,
};
} else {
return {
element: meta.obj,
};
}
}
}

export default {
preprocessDataLayer(layer, view) {
if (!layer.file) {
Expand Down Expand Up @@ -231,6 +248,9 @@ export default {
layer.update = PointCloudProcessing.update;
layer.postUpdate = PointCloudProcessing.postUpdate;

// override the default method, since updated objects are metadata in this case
layer.getObjectToUpdateForAttachedLayers = getObjectToUpdateForAttachedLayers;

// this probably needs to be moved to somewhere else
layer.pickObjectsAt = (view, mouse, radius) => Picking.pickPointsAt(view, mouse, radius, layer);

Expand All @@ -253,31 +273,32 @@ export default {

executeCommand(command) {
const layer = command.layer;
const node = command.requester;
const metadata = command.requester;

// Query HRC if we don't have children metadata yet.
if (node.childrenBitField && node.children.length === 0) {
parseOctree(layer, layer.metadata.hierarchyStepSize, node).then(() => command.view.notifyChange(layer, false));
if (metadata.childrenBitField && metadata.children.length === 0) {
parseOctree(layer, layer.metadata.hierarchyStepSize, metadata).then(() => command.view.notifyChange(layer, false));
}

// `isLeaf` is for lopocs and allows the pointcloud server to consider that the current
// node is the last one, even if we could subdivide even further.
// It's necessary because lopocs doens't know about the hierarchy (it generates it on the fly
// when we request .hrc files)
const url = `${node.baseurl}/r${node.name}.${layer.extension}?isleaf=${command.isLeaf ? 1 : 0}`;
const url = `${metadata.baseurl}/r${metadata.name}.${layer.extension}?isleaf=${command.isLeaf ? 1 : 0}`;

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;
points.matrixAutoUpdate = false;
points.position.copy(node.bbox.min);
points.position.copy(metadata.bbox.min);
points.scale.set(layer.metadata.scale, layer.metadata.scale, layer.metadata.scale);
points.updateMatrix();
points.tightbbox = geometry.boundingBox.applyMatrix4(points.matrix);
points.layers.set(layer.threejsLayer);
points.layer = layer;
points.extent = Extent.fromBox3(command.view.referenceCrs, node.bbox);
points.extent = Extent.fromBox3(command.view.referenceCrs, metadata.bbox);
points.userData.metadata = metadata;
return points;
});
},
Expand Down
Loading