Skip to content

chore(pointcloud): Add adaptive size mode#2666

Open
ketourneau wants to merge 14 commits into
iTowns:masterfrom
bloc-in-bloc:f/adaptive_size_mode
Open

chore(pointcloud): Add adaptive size mode#2666
ketourneau wants to merge 14 commits into
iTowns:masterfrom
bloc-in-bloc:f/adaptive_size_mode

Conversation

@ketourneau

Copy link
Copy Markdown
Contributor

Description

We try to implement adaptive size mode.

Motivation and Context

Our customer needs it to draw on pointcloud.

Screenshots (if appropriate)

Enregistrement.de.l.ecran.2025-12-17.a.08.51.43.mov

@ketourneau

Copy link
Copy Markdown
Contributor Author

I see an issue with postUpdate, pointcloud size is wrong at initialisation (postUpdate is called before points loaded) : #2665

@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch 3 times, most recently from d124dc9 to 31b11dc Compare February 3, 2026 09:01
@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch 6 times, most recently from 40b5fb7 to 90df547 Compare March 11, 2026 08:57
@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch 8 times, most recently from 75ba231 to 1fb2419 Compare March 17, 2026 13:35
Comment thread packages/Main/src/Layer/PointCloudLayer.ts
@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch from 1fb2419 to aae36f4 Compare March 19, 2026 15:26
Comment thread packages/Main/src/Layer/PointCloudLayer.ts Outdated
@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch 6 times, most recently from 5a70f87 to cf37835 Compare March 24, 2026 10:46
@jailln

jailln commented May 13, 2026

Copy link
Copy Markdown
Contributor

@ketourneau nice addition thanks ! Tested ok on 3D Tiles datasets 🙂

I think we should modify the point clouds examples (including 3dtiles_loader.html) to use this display mode. WDYT @Desplandis ?

@jailln

jailln commented May 19, 2026

Copy link
Copy Markdown
Contributor

@ketourneau nice addition thanks ! Tested ok on 3D Tiles datasets 🙂

I think we should modify the point clouds examples (including 3dtiles_loader.html) to use this display mode. WDYT @Desplandis ?

After discussion with @ketourneau it is not applied to 3D Tiles layers because the implementation is based on the assumption that the data is organized in an octree. I'm not familiar enough with this algorithm to know if and how it could be adapted to 3D tiles but it should not block this PR. We should however add in the doc and maybe in a warning that it is not supported for 3D Tiles layers.

@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch from 609b339 to 22bbf6e Compare May 19, 2026 12:53
@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch from 22bbf6e to b7766c6 Compare June 3, 2026 08:42
@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch from b7766c6 to baef75b Compare June 10, 2026 09:40
@ftoromanoff

Copy link
Copy Markdown
Contributor

Small remarks for the PointCloudDebug. The 'Min size' and 'Max size' are not use for the 'new 'ADAPTIVE' 'Size mode', are they ? In this case the should be hidden as for when we set the 'Size mode' on 'VALUE'.

Comment on lines +556 to +557
const data = vnt.image.data;
data.set(this._visibilityTextureData.data);

@Desplandis Desplandis Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const data = vnt.image.data;
data.set(this._visibilityTextureData.data);
const data = vnt.image.data;
data.fill(0);
data.set(this._visibilityTextureData.data);

I think there are still some stale entries from the previous updates, I recommend you to zero out all the buffer. It is weird however that we access those part of the buffer but it shall fix the issue for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, good point.

for (const pts of this.group.children) {
const node = pts.userData.node;
const depth = node.depth;
const nodeStartOffset = this._visibilityTextureData.nodeToIndex.get(node);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Beware that nodeStartOffset may be undefined! This is can happen because the rendered group this.group.children do not guarantee that all points are visible. They are in the rendered group (were visible once) but remain hidden until disposal (10s later).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok I continue to next iteration if nodeStartOffset is undefined

float nodeSizeAtLevel = octreeSize / pow(2.0, i + nodeDepth);

vec3 index3d = (pos - offset) / nodeSizeAtLevel;
index3d = floor(index3d + 0.5);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the more important issue of all. It seems that index3d may be outside the range $[0, 1]$. Clamping here fix the big points issues. It may be because of floating point error accumulation or because we are outside the bounds...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I applied your suggestion.

index3d = floor(index3d + 0.5);
int index = int(round(4.0 * index3d.x + 2.0 * index3d.y + index3d.z));

vec4 value = texture2D(visibleNodes, vec2(float(iOffset) / 2048.0, 0.0));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
vec4 value = texture2D(visibleNodes, vec2(float(iOffset) / 2048.0, 0.0));
vec4 value = texture2D(visibleNodes, vec2((float(iOffset) + 0.5) / 2048.0, 0.5));

I suggest to sample at mid-pixel in case of a small floating-point error to not sample the neighbour texel.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I applied your suggestion.

@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch from baef75b to 1938853 Compare June 18, 2026 08:41
@ketourneau

Copy link
Copy Markdown
Contributor Author

Small remarks for the PointCloudDebug. The 'Min size' and 'Max size' are not use for the 'new 'ADAPTIVE' 'Size mode', are they ? In this case the should be hidden as for when we set the 'Size mode' on 'VALUE'.

Yes, indeed, I had asked myself the same question. I hided the 'Min size' and 'Max size'

@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch 2 times, most recently from cbd072a to fde42d5 Compare June 18, 2026 14:57
Comment thread packages/Debug/src/PointCloudDebug.js Outdated
@@ -299,7 +299,7 @@ export default {
styleUI.add(layer, 'opacity', 0, 1).name('Layer opacity').onChange(update);
styleUI.add(layer, 'pointSize', 0, 15).name('Point size').onChange(update);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This parameter initialy only have an impact on Size mode = VALUE (and not when Size mode = ATTENUATED) thus it should be put under 'Size mode' and be hidden when we are 'ATTENUATED' ?
Now i see that in the mode = ADAPTIVE, it's also has an impact even if i don't really understand to what exactly the value will refer to.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In ADAPTIVE mode this parameter can be use to apply a scale factor. But normally, 1.0 factor value should be enough in most cases

@ftoromanoff ftoromanoff Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would then still suggest to move this parameter after the 'Size mode' button and hide it when it has no impact. It is just purely ergonic and not algorythmic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok done

@ftoromanoff ftoromanoff Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's fine by me. I don't validate the PR as I didn't review the rest of the code. I'll let @Desplandis do that.

@ketourneau ketourneau force-pushed the f/adaptive_size_mode branch from e5fbf4c to ab341b2 Compare June 30, 2026 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants