Skip to content

Fix(pointcloud): incorrect loading mechanism & add layer events #2673

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

Merged
Desplandis merged 2 commits into from
Jan 28, 2026
+26 −81
Merged

Fix(pointcloud): incorrect loading mechanism & add layer events #2673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4733d0c
fix(pointcloud): fix node loading mechanism
Desplandis Jan 15, 2026
e25e7f9
feat(pointcloud): add load, error and dispose events
Desplandis Jan 16, 2026
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
63 changes: 25 additions & 38 deletions packages/Main/src/Layer/PointCloudLayer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
}

export interface PointCloudLayerParameters {
/** Description and options of the source See @Layer. */

Check warning on line 21 in packages/Main/src/Layer/PointCloudLayer.ts

View workflow job for this annotation

GitHub Actions / Build bundle, check Linter and generate documentation 

tsdoc-characters-after-block-tag: The token "@layer" looks like a TSDoc tag but contains an invalid character "."; if it is not a tag, use a backslash to escape the "@"
source: PointCloudSource;
object3d?: THREE.Group;
group?: THREE.Group;
Expand Down Expand Up @@ -67,7 +67,9 @@
earlyDropFunction?: (cmd: { requester: PointCloudNode }) => boolean;
}) => Promise<THREE.Points>;
};
view: object;
view: {
notifyChange: (elt: PointCloudLayer) => void;
};
}

function computeSSEPerspective(
Expand Down Expand Up @@ -323,8 +325,7 @@
this.maxElevationRange = this.maxElevationRange ?? this.source.zmax;
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
preUpdate(context: Context, changeSources: any) {
preUpdate(context: Context) {
// See https://cesiumjs.org/hosted-apps/massiveworlds/downloads/Ring/WorldScaleTerrainRendering.pptx
// slide 17
context.camera.preSSE =
Expand All @@ -344,36 +345,6 @@
}
}

// lookup lowest common ancestor of changeSources
let commonAncestor;
for (const source of changeSources.values()) {
if (source.isCamera || source == this) {
// if the change is caused by a camera move, no need to bother
// to find common ancestor: we need to update the whole tree:
// some invisible tiles may now be visible
return [this.root];
}
if (source.obj === undefined) {
continue;
}
// filter sources that belong to our layer
if (source.obj.isPoints && source.obj.layer == this) {
if (!commonAncestor) {
commonAncestor = source;
} else {
commonAncestor = source.findCommonAncestor(commonAncestor);

if (!commonAncestor) {
return [this.root];
}
}
}
}

if (commonAncestor) {
return [commonAncestor];
}

Copy link
Contributor

@ftoromanoff ftoromanoff Jan 20, 2026

Choose a reason for hiding this comment

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

Are we sure we want to removed that part (as you said it's never called) but an other solution might have been to try to make this part used.

Copy link
Contributor Author

@Desplandis Desplandis Jan 22, 2026

Choose a reason for hiding this comment

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

I would prefer to remove unused (pre-mature) optimisation for now, until we manage to have a perfectly stable implementation of the point cloud update mechanism. Moreover, I do not use this code in the future priority-based mechanism.

Copy link
Contributor

@ftoromanoff ftoromanoff Jan 23, 2026

Choose a reason for hiding this comment

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

I see, but we might ask @jailln and @gchoqueux to agree ? Just to be sure ?

Copy link
Contributor

@jailln jailln Jan 26, 2026

Choose a reason for hiding this comment

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

Ok for me to remove it

// Start updating from hierarchy root
return [this.root];
}
Expand Down Expand Up @@ -404,7 +375,12 @@
} else if (!elt.promise) {
const distance = Math.max(0.001, distanceToCamera);
// Increase priority of nearest node
const priority = computeScreenSpaceError(context, layer.pointSize, elt.pointSpacing, distance) / distance;
const priority = computeScreenSpaceError(
context,
layer.pointSize,
elt.pointSpacing,
distance,
) / distance;
elt.promise = context.scheduler.execute({
layer,
requester: elt,
Expand All @@ -414,12 +390,15 @@
earlyDropFunction: cmd => !cmd.requester.visible || !this.visible,
}).then((pts: THREE.Points) => {
elt.obj = pts;

elt.obj.visible = false;
// make sure to add it here, otherwise it might never
// be added nor cleaned
this.group.add(elt.obj);
elt.obj.updateMatrixWorld(true);
context.view.notifyChange(this);
this.dispatchEvent({ type: 'load-model', scene: pts, tile: elt });
Copy link
Contributor

@ftoromanoff ftoromanoff Jan 20, 2026

Choose a reason for hiding this comment

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

Currently this event type is only listened in OGC3DTilesLayer.js and OGC3DTilesDebug.js.
And I don't see here any addEventListener for these... Is it really useful to add them then ?

Copy link
Contributor Author

@Desplandis Desplandis Jan 22, 2026

Choose a reason for hiding this comment

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

I use them for debugging purpose and I think this would be a nice addition to users who want to change the material (model-load) or just if we wish to implement some sort of benchmarking.

Copy link
Contributor

@jailln jailln Jan 22, 2026

Choose a reason for hiding this comment

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

agreed with @Desplandis , always useful for users to have access to such events

Copy link
Contributor

@ftoromanoff ftoromanoff Jan 23, 2026

Choose a reason for hiding this comment

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

Ok, I just currently don't see any use of them, we just need to be sure that they will be documented ;-)

Copy link
Contributor

@jailln jailln Jan 26, 2026

Choose a reason for hiding this comment

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

Yes they indeed need to be documented :)

Copy link
Contributor

@ftoromanoff ftoromanoff Jan 29, 2026

Choose a reason for hiding this comment

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

@Desplandis Was this done ?

}).catch((err: { isCancelledCommandException: boolean }) => {
this.dispatchEvent({ type: 'load-error', tile: elt, error: err });
if (!err.isCancelledCommandException) {
return err;
}
Expand All @@ -430,7 +409,12 @@
}

if (elt.children && elt.children.length) {
elt.sse = computeScreenSpaceError(context, layer.pointSize, elt.pointSpacing, distanceToCamera) / this.sseThreshold;
elt.sse = computeScreenSpaceError(
context,
layer.pointSize,
elt.pointSpacing,
distanceToCamera,
) / this.sseThreshold;
if (elt.sse >= 1) {
return elt.children;
} else {
Expand Down Expand Up @@ -499,7 +483,8 @@
}

if (this.displayedCount > this.pointBudget) {
// 2 different point count limit implementation, depending on the potree source
// 2 different point count limit implementation, depending on the
// potree source
if (this.supportsProgressiveDisplay) {
// In this format, points are evenly distributed within a node,
// so we can draw a percentage of each node and still get a
Expand Down Expand Up @@ -543,9 +528,11 @@
// remove from group
this.group.children.splice(i, 1);

// no need to dispose obj.material, as it is shared by all objects of this layer
// no need to dispose obj.material, as it is shared by all
// objects of this layer
obj.geometry.dispose();
obj.userData.node.obj = null;
this.dispatchEvent({ type: 'dispose-model', scene: obj, tile: obj.userData.node });
}
}
}
Expand Down
23 changes: 1 addition & 22 deletions packages/Main/test/unit/potree2layerprocessing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,33 +58,12 @@ describe('preUpdate Potree2Layer', function () {
Potree2Layer.prototype.preUpdate.call(layer, context, sources)[0]);
});

it('should return root if no common ancestors', () => {
it('should return root', () => {
const sources = new Set();
sources.add(layer.root.children[0].children[0]);
sources.add(layer.root.children[2].children[1]);
assert.deepStrictEqual(
layer.root,
Potree2Layer.prototype.preUpdate.call(layer, context, sources)[0]);
});

it('should return common ancestor', () => {
const sources = new Set();
sources.add(layer.root.children[2].children[0]);
sources.add(layer.root.children[2].children[1]);
sources.add(layer.root.children[2].children[1].children[2]);
sources.add(layer.root.children[2].children[1].children[3]);
assert.deepStrictEqual(
layer.root.children[2],
Potree2Layer.prototype.preUpdate.call(layer, context, sources)[0]);
});

it('should not search ancestors if layer are different root if no common ancestors', () => {
const sources = new Set();
sources.add(layer.root.children[2].children[0]);
sources.add(layer.root.children[2].children[1].children[3]);
layer.root.children[2].children[1].children[3].obj = { layer: {}, isPoints: true };
assert.deepStrictEqual(
layer.root.children[2].children[0],
Potree2Layer.prototype.preUpdate.call(layer, context, sources)[0]);
});
});
21 changes: 0 additions & 21 deletions packages/Main/test/unit/potreelayerprocessing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,25 +66,4 @@ describe('preUpdate PotreeLayer', function () {
layer.root,
PotreeLayer.prototype.preUpdate.call(layer, context, sources)[0]);
});

it('should return common ancestor', () => {
const sources = new Set();
sources.add(layer.root.children[2].children[0]);
sources.add(layer.root.children[2].children[1]);
sources.add(layer.root.children[2].children[1].children[2]);
sources.add(layer.root.children[2].children[1].children[3]);
assert.deepStrictEqual(
layer.root.children[2],
PotreeLayer.prototype.preUpdate.call(layer, context, sources)[0]);
});

it('should not search ancestors if layer are different root if no common ancestors', () => {
const sources = new Set();
sources.add(layer.root.children[2].children[0]);
sources.add(layer.root.children[2].children[1].children[3]);
layer.root.children[2].children[1].children[3].obj = { layer: {}, isPoints: true };
assert.deepStrictEqual(
layer.root.children[2].children[0],
PotreeLayer.prototype.preUpdate.call(layer, context, sources)[0]);
});
});