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.

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(PointCloud): increase range of picking to u32 #2288

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HoloTheDrunk
Copy link
Contributor

@HoloTheDrunk HoloTheDrunk commented Mar 11, 2024

Description

Change the picking IDs from u16 to u32.

Motivation and Context

Effectively eliminates the possibility of not having enough IDs to perform picking until GPUs start having 32GB+ of VRAM.
Tested on Firefox 123.0 (64-bit) on Ubuntu Linux 22.04.4 LTS x86_64

@HoloTheDrunk HoloTheDrunk force-pushed the larger_cloud_picking branch from 66c2e5d to 49d41be Compare March 11, 2024 16:28
@HoloTheDrunk
Copy link
Contributor Author

I don't know where the functional test errors are coming from, I can't replicate them locally so I'm guessing (hoping) it's a toolchain problem and not on my end

@HoloTheDrunk HoloTheDrunk force-pushed the larger_cloud_picking branch from 49d41be to 4c5e66c Compare March 13, 2024 12:57
@HoloTheDrunk HoloTheDrunk force-pushed the larger_cloud_picking branch from 4c5e66c to 58a43d0 Compare March 13, 2024 13:09
@Desplandis Desplandis self-requested a review June 24, 2024 13:34
Comment on lines +151 to +155
const data = buffer.slice(idx, idx + 3);

// see PotreeProvider and the construction of unique_id
const objId = (data[0] << 8) | data[1];
const index = (data[2] << 8) | data[3];
const objId = data[0];
const index = (data[1] << 16) | data[2];
Copy link
Contributor

Choose a reason for hiding this comment

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

You encode each 48-bits unique_id value as 3 uint16. However, you read the resulting texture with a uint8 buffer (see c3dEngine#renderViewToBuffer)...

I don't think values are preserved here.

Copy link
Contributor Author

@HoloTheDrunk HoloTheDrunk Jun 25, 2024

Choose a reason for hiding this comment

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

We can provide a u16 buffer ourselves

- const buffer = view.mainLoop.gfxEngine.renderViewToBuffer(
-     { camera: view.camera, scene: tileLayer.object3d },
-     {
-         x: viewCoords.x - radius,
-         y: viewCoords.y - radius,
-         width: 1 + radius * 2,
-         height: 1 + radius * 2,
-     });
+ const zone = {
+     x: viewCoords.x - radius,
+     y: viewCoords.y - radius,
+     width: 1 + radius * 2,
+     height: 1 + radius * 2,
+ };
+ zone.buffer = new Uint16Array(4 * zone.width * zone.height);
+ const buffer = view.mainLoop.gfxEngine.renderViewToBuffer(
+     { camera: view.camera, scene: tileLayer.object3d },
+     zone,
+ );

But looking into things a bit more I'm not sure how this is gonna interact with the default fullSizeRenderTarget used by the c3DEngine that gets created with u8 rgba formatting.
I currently don't have an example I could use to check for this issue, as the results don't change for the example I originally used for this PR (potree 3d map).

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.

2 participants