-
Notifications
You must be signed in to change notification settings - Fork 100
Gaussians LOD #189
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
base: master
Are you sure you want to change the base?
Gaussians LOD #189
Conversation
…ng will be done as points or splats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .DS_Store files cannot be committed to this repo. The rest is good tho 👍
buffer = await response.arrayBuffer(); | ||
} | ||
|
||
const pointAttributes = node.octreeGeometry.pointAttributes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of formatting issue in this repo, probably because there is no CI to check that automatically. I'll just make a PR where I run prettier once this one is merged
|
||
*/ | ||
|
||
const buffer = toUint8Array('AGFzbQEAAAAADwhkeWxpbmsuMAEEAAAAAAEPAmAAAGAIf39/f39/f38AAg8BA2VudgZtZW1vcnkCAAADAwIAAQcjAhFfX3dhc21fY2FsbF9jdG9ycwAAC3NvcnRJbmRleGVzAAEKhgMCAwABC/8CAgN/A30gBwRAIAQqAighCyAEKgIYIQwgBCoCCCENQfj///8HIQlBiICAgHghBANAIAIgCkECdCIIaiALIAEgACAIaigCAEEEdGoiCCoCCJQgDSAIKgIAlCAMIAgqAgSUkpJDAACARZT8ACIINgIAIAkgCCAIIAlKGyEJIAQgCCAEIAhKGyEEIApBAWoiCiAHRw0ACyAGQQFrsyAEsiAJspOVIQtBACEEA0AgAiAEQQJ0aiIBIAsgASgCACAJa7KU/AAiATYCACADIAFBAnRqIgEgASgCAEEBajYCACAEQQFqIgQgB0cNAAsLIAZBAk8EQCADKAIAIQlBASEEA0AgAyAEQQJ0aiIBIAEoAgAgCWoiCTYCACAEQQFqIgQgBkcNAAsLIAdBAEoEQCAHIQQDQCAFIAcgAyACIARBAWsiAUECdCIGaigCAEECdGoiCSgCACIIa0ECdGogACAGaigCADYCACAJIAhBAWs2AgAgBEEBSyEGIAEhBCAGDQALCws=') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably that it would be easier to maintain to let webpack inject the wasm in the generated code when building the library. I found an example here where it intanciate the Webasembly on its own: https://github.com/ballercat/minimal-webpack5-wasm-demo/blob/main/webpack.config.js, and a simpler one here where webpack inject it as a string. I both case, the main benefit is that you don't have to copy pat code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah exactly. Or remove the wasm2js as a dependency since today it's not used during runtime, nor in the build process.
remove the .DS_Store file that went through the PR
remove the .DS_Store that was committed by mistake
example/main.ts
Outdated
|
||
//pco.rotateX(-Math.PI * 0.5); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//pco.rotateX(-Math.PI * 0.5); | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
file: 'cloud.js', | ||
url: 'https://raw.githubusercontent.com/potree/potree/develop/pointclouds/lion_takanawa/', | ||
version: 'v1' | ||
}, { | ||
file: 'metadata.json', | ||
url: 'https://test-pix4d-cloud-eu-central-1.s3.eu-central-1.amazonaws.com/lion_takanawa_converted/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't look right. The v1 should still be in potree v1.
When I launch example, I cannot load the first model. In the console I get this error:
main.ts:91 TypeError: Failed to fetch
at xhrRequest (potree.ts:82:1)
at OctreeLoader.fetchMetadata (octree-loader.ts:264:1)
at OctreeLoader.load (octree-loader.ts:251:1)
at Potree.loadOctree [as loadGeometry] (load-octree.ts:5:1)
eval @ main.ts:91
Promise.catch
setupPointCloud @ main.ts:91
eval @ main.ts:119
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in general I see a few issues with the example. I got another error when I moved the second slider
main.ts:61 Uncaught TypeError: Cannot set properties of undefined (setting 'value')
at HTMLInputElement.eval (main.ts:61:1)
eval @ main.ts:61
Could we add some labels to the sliders to understand what they're doing? It would also be great to have both models described somehow so that we know what we're loading in the ui.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
scripts/compile_wasm_script.sh
Outdated
@@ -0,0 +1 @@ | |||
em++ -std=c++11 sorter_no_simd.cpp -Os -s WASM=1 -s SIDE_MODULE=2 -o sorter_no_simd_non_shared.wasm -s IMPORTED_MEMORY=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a target shell notation to the script:
https://www.shellcheck.net/wiki/SC2148
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
scripts/compile_wasm_script.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant name "_script".
nitpick: I'm wondering if it wouldn't be better to put it in under "scripts" section in the package.json file.
Should this be a part of the building process? Or under what circumstances this should be helpful again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed, it shouldn´t be too helpful since I do not think people will compile the sorter again, but I left it there if anyone wants to test how to compile the C sorter.
@@ -5,9 +5,10 @@ export async function loadOctree( | |||
url: string, | |||
getUrl: GetUrlFn, | |||
xhrRequest: XhrRequest, | |||
loadHarmonics: boolean = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are using 2 different names for the feature. Should it be loadGaussians or loadHarmonics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, it was an issue with the loaders that is fixed now, it only has to take into account if it load the harmonics (for the V2 version)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example shoudl work before we merge anything.
I would also like to take a look at testing. What can we effectively test here and how?
Setting up test is not super straightforward as we have no CI for this repo. So either we set up one, we agree on manually running UTs, or we run them in pre-commit hook. |
rotations[4 * j + 3] = tempRotation.w; | ||
} | ||
|
||
attributeBuffers["orientation"] = { buffer: bufferRotations, attribute: "orientation" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some attributes are local static string and some others are defined in the enum PointAttibute. It would be better to have them all in the enum
I can setup a CI to run tests against this repo. |
Adding the gaussians splats rendering with LOD to Potree, has the new features: