Skip to content

Conversation

d0rianb
Copy link

@d0rianb d0rianb commented Sep 24, 2024

For now, the viewer can handle URL which can be downloaded from with the fetch API.
I wanted to add the possibility to load a scene directly from a file, to handle drag & drop for instance.

Some features are still missing, like the progressive loading, or multiple files loading. I plan on integrate these soon.

Moreover, I struggled a bit on the 'abortable' promise that is returned by downloadAndBuildSingleSplatSceneStandardLoad. I couldn't manage to get the differences between the two following lines :

const downloadPromise = this.downloadSplatSceneToSplatBuffer(path, splatAlphaRemovalThreshold, onProgress, false, undefined, format);
const downloadAndBuildPromise = abortablePromiseWithExtractedComponents(downloadPromise.abortHandler);

import { SplatBuffer } from '../SplatBuffer.js';
import { PlyParserUtils } from './PlyParserUtils.js';

const BaseFieldNamesToRead = ['scale_0', 'scale_1', 'scale_2', 'rot_0', 'rot_1', 'rot_2', 'rot_3', 'x', 'y', 'z',

Choose a reason for hiding this comment

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

applying prettifier to unchanged lines clutters reviewing the PR :)

package.json Outdated
"three": ">=0.160.0"
},
"dependencies": {
"@aws-sdk/client-s3": "^3.750.0"
Copy link

@hirako2000 hirako2000 Mar 13, 2025

Choose a reason for hiding this comment

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

Why add the (bloated) client-s3 library? Is that relevant to loading from a file?

Copy link
Author

@d0rianb d0rianb Mar 16, 2025

Choose a reason for hiding this comment

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

Hi, it's not relevant for this PR, i kept working on this branch to add remote loading from R2 buckets, but did not realise the commits will be uploaded to this PR. I will clean this.

Choose a reason for hiding this comment

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

Happy to help test and review this, it's a great PR.

@d0rianb
Copy link
Author

d0rianb commented Mar 17, 2025

The changes should be those of the dev branch now, apart from the specific ones related to loading a file or a promise

int maxDistance = -2147483640;
int minDistance = 2147483640;

if (distanceMapRange > sortCount) distanceMapRange = sortCount;

Choose a reason for hiding this comment

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

Seems like an unrelated change here.

int minDistance = 2147483640;

if (distanceMapRange > sortCount) distanceMapRange = sortCount;

Choose a reason for hiding this comment

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

here too.

document.body.appendChild(this.rootElement);
} else {
this.rootElement = this.renderer.domElement || document.body;
this.rootElement = this.renderer.domElement.parentElement || document.body;

Choose a reason for hiding this comment

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

Not sure they could always be a parent element. In any case this seems unrelated to the changeset.

src/Viewer.js Outdated
setupCamera() {
if (!this.usingExternalCamera) {
const renderDimensions = new THREE.Vector2();
this.getRenderDimensions(renderDimensions);

Choose a reason for hiding this comment

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

is this refactor necessary? Is that fixing something related to processing files? It doesn't seem to be.

} else if (format === SceneFormat.Spz) {
return SpzLoader.loadFromURL(path, onProgress, splatAlphaRemovalThreshold, this.inMemoryCompressionLevel,
this.optimizeSplatData, this.sphericalHarmonicsDegree, headers);
if (format === SceneFormat.Splat) {

Choose a reason for hiding this comment

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

Nice refactor, but see the headers got dropped. These are used by down the stack as below. I would keep the refactor, but continue to pass the headers options.

if (headers) fetchOptions.headers = headers;
         fetch(path, fetchOptions)
        .then(async (data) => {
            // Handle error conditions where data is still returned
            if (!data.ok) {
                const errorText = await data.text();
                reject(new Error(`Fetch failed: ${data.status} ${data.statusText} ${errorText}`));
                return;
            }

src/Viewer.js Outdated
loadSplatSceneFromFileToSplatBuffer(fileData, splatAlphaRemovalThreshold = 1, onProgress = undefined,
progressiveBuild = false, onSectionBuilt = undefined, format) {
if (format === SceneFormat.Splat) {
return SplatLoader.loadFromFileData(fileData, splatAlphaRemovalThreshold, 0, false);

Choose a reason for hiding this comment

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

why not passing the compression level? rather than magic 0?

Also, I suggest this function to handle inMemoryCompressionLevel as an option parameter with some default.

See that the viewer otherwise (URL loading feature) supports it as an option.


static loadFromURL(fileName, onProgress, minimumAlpha, compressionLevel, optimizeSplatData = true,
outSphericalHarmonicsDegree = 0, headers, sectionSize, sceneCenter, blockSize, bucketSize) {
const localOnProgress = (percent, percentLabel) => {

Choose a reason for hiding this comment

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

I'm unsure of the possible side effect there, but this change at least obscures how the onProgress eventing functions, which already is not trivial to grasp.

I even doubt this change is necessary.

@@ -1 +1 @@
em++ -std=c++11 sorter_no_simd.cpp -Os -s WASM=1 -s SIDE_MODULE=2 -o sorter_no_simd.wasm -s IMPORTED_MEMORY=1 -s SHARED_MEMORY=1
em++ -std=c++11 sorter_no_simd.cpp -Os -s WASM=1 -s SIDE_MODULE=2 -o sorter_no_simd.wasm -s IMPORTED_MEMORY=1 -s USE_PTHREADS=1

Choose a reason for hiding this comment

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

was it some troubleshooting changes? is that needed?

I would not touch this given the no_simd is likely a script for compatibility sake. Not sure tbh.

@hirako2000
Copy link

the branch is now far more readable, thanks for the clean up.

I would suggest updating the README with how to use this. I will test it then.

@d0rianb d0rianb changed the base branch from main to dev March 19, 2025 07:47
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.

3 participants