Skip to content

[Refactor] Point Cloud Nodes -> homogeneisation#2687

Open
ftoromanoff wants to merge 17 commits intoiTowns:masterfrom
ftoromanoff:refactor/pointCloud
Open

[Refactor] Point Cloud Nodes -> homogeneisation#2687
ftoromanoff wants to merge 17 commits intoiTowns:masterfrom
ftoromanoff:refactor/pointCloud

Conversation

@ftoromanoff
Copy link
Contributor

@ftoromanoff ftoromanoff commented Jan 27, 2026 •
edited
Loading

Light refactorisation of Point Cloud Node and fix.

  • PointCloudDebug :

    • remove BoxHelper to only use OBBHelper
    • fix a bug linked with custom projection

  • PointCloud refactor

    • remove all pointNumber Default value (and keep -1 when node is not loaded yet)
    • load Octree in node only

  • refactorisation of Potree and Potree2 and fix for potree

  • Proposal of a new structure for PointCloudNode test:

    • add a pointcloudnode.js test file
    • move node test from generic PointCloudType files to their own pointCloudNode files
    • regroup them all in a folder node

    The commit with the Potree2 Arene data will be removed, but I let it for test on Potree2 if needed

@ftoromanoff ftoromanoff marked this pull request as ready for review January 27, 2026 17:54
@ftoromanoff ftoromanoff force-pushed the refactor/pointCloud branch 10 times, most recently from 6827bb5 to faf41ae Compare January 29, 2026 13:49
@ftoromanoff ftoromanoff self-assigned this Jan 29, 2026
@ftoromanoff ftoromanoff changed the title (Refactor): point cloud node [Refactor] Point Cloud Nodes -> homogeneisation Jan 29, 2026
@ftoromanoff ftoromanoff mentioned this pull request Feb 3, 2026
Closed
@ftoromanoff ftoromanoff force-pushed the refactor/pointCloud branch 13 times, most recently from c595be4 to fb61387 Compare February 11, 2026 14:02
@ftoromanoff ftoromanoff force-pushed the refactor/pointCloud branch 2 times, most recently from c595be4 to fb61387 Compare February 11, 2026 14:02
Desplandis
Desplandis reviewed Feb 17, 2026
View reviewed changes
Copy link
Contributor

@Desplandis Desplandis left a comment

Choose a reason for hiding this comment

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

Those are my comment for now. As for the tests, I was thinking that you had rewrote them to not depend on external data sources (using sinon)? Or maybe is this in another PR?

Note that when using sinon, you could put all your stub in a sandbox (see https://sinonjs.org/releases/latest/sandbox/), that way we could restore the context and avoid side-effects!

packages/Debug/src/PointCloudDebug.js

const red = new THREE.Color(0xff0000);
// const blue = new THREE.Color(0x0000ff);
const yellow = new THREE.Color(0xffff00);
Copy link
Contributor

@Desplandis Desplandis Feb 16, 2026

Choose a reason for hiding this comment

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

I implemented an event-driven tool so you'll have difficulty to rebase. However I'm interested in some of the changes here!

packages/Main/src/Parser/PotreeBinParser.js Outdated
Comment on lines +138 to +142
const center = new Coordinates(options.in.crs)
.setFromVector3(options.in.clampOBB.center);
const centerZ0 = projZ0(center, source.crs, options.in.crs);
const quaternion = getQuaternion(centerZ0, source.crs, options.in.crs);
const quaternionArr = quaternion.toArray();
Copy link
Contributor

@Desplandis Desplandis Feb 17, 2026

Choose a reason for hiding this comment

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

I don't understand this code. The clampOBB is already in the target coordinates reference system, why should we need to project it again if we need the "center" (at z = 0)? I don't understand why we can't just take the center of the bbox multiplied by the matrix of the OBB? Are we sure that this optimisation gives us more precision?

This comment also applies for the similar code in LASParser & Potree2BinParser.

Copy link
Contributor Author

@ftoromanoff ftoromanoff Feb 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

There is 2 things:

  1. We want to use as origin for the parsing, a value at z=0, on the origin crs, to be able to directly have access to the elevation in a point.
  2. I recompute the center of the OBB, as I consider that a projection add a 'deformation' to space and not only a rotation and transalation. Thus the center of the OBB in the native crs and on the projected crs will not be the same.

If we want to make abstraction of the 'deformation' we can indeed just use
new THREE.Vector3().applyMatrix4(options.in.clampOBB.matrixWorld
as the clampedOBB geometry has already been centered (thus x0 et y0 are at the center) and as we want z to be at 0 as well.
It will simplified the code, should I use it ?

Copy link
Contributor Author

@ftoromanoff ftoromanoff Feb 19, 2026

Choose a reason for hiding this comment

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

I did use the simplification

packages/Main/src/Parser/LASParser.js Outdated
geometry.boundingBox = new THREE.Box3().fromJSON(parsedData.box);
geometry.userData.header = parsedData.header;

const geometry = await parse(data, options, 'parseFile');
Copy link
Contributor

@Desplandis Desplandis Feb 17, 2026

Choose a reason for hiding this comment

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

I would prefer for typing reasons to have some functions shared by parseFile and parseChunk. One for synthesising the common parameters from options, one for setting the buffer geometry and boundingBoxes, position, quaternion.

Copy link
Contributor Author

@ftoromanoff ftoromanoff Feb 20, 2026

Choose a reason for hiding this comment

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

I did change it, Does it correspond more at what you had in mind ? @Desplandis

@ftoromanoff
Copy link
Contributor Author

ftoromanoff commented Feb 17, 2026

Those are my comment for now. As for the tests, I was thinking that you had rewrote them to not depend on external data sources (using sinon)? Or maybe is this in another PR?

Note that when using sinon, you could put all your stub in a sandbox (see https://sinonjs.org/releases/latest/sandbox/), that way we could restore the context and avoid side-effects!

In this PR, I propose a new organisation of the pointcloudnode test, but I rewrote them in another PR...

@ftoromanoff
refactor(LASParser): refactor commun code
f24a762
@ftoromanoff
refactor(PointCloudDebug): changed BoxHelper to OBBHelper
0edb125
@ftoromanoff
fix(PointCloudDebug): custom reprojection and OBB
c6489ba
@ftoromanoff
refactor(PointCloudNode): remove numPoints default value
a3feb41
@ftoromanoff
refactor(PointCloud): supp call to loadOctree() in Layer
1250bb1
@ftoromanoff
fix(Potree): fix and simplification-> using OBB.natBox (src.bounds)
d33657f
@ftoromanoff
refactor(Potree2): simplification of code
95ddb30
@ftoromanoff
refactor(PointCloudNode): move calcul of origin and rotation to parser
055381a
@ftoromanoff
test(PointCloudParser): fix
e03cf82
@ftoromanoff
fix(test): point cloud test files
c2eda27
@ftoromanoff
example(PointCloud): update and cleanup
383f3b9
@ftoromanoff
examples(Potree2): add new exemples Potree2 3D with valid data //DO N…
27a11ad 
…OT PUSH into MASTER
@ftoromanoff
test(pointcloudnode): refactor PointCloudNode test
f0cdab6
@ftoromanoff ftoromanoff mentioned this pull request Mar 3, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Desplandis Desplandis Desplandis left review comments

@jailln jailln Awaiting requested review from jailln

At least 1 approving review is required to merge this pull request.

Assignees

@ftoromanoff ftoromanoff

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ftoromanoff @Desplandis