Skip to content

Refactoring Loader #2641

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.

Sign up for GitHub

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

Jump to bottom
Open
gchoqueux wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Refactoring Loader #2641

gchoqueux wants to merge 2 commits into from

Conversation

@gchoqueux
Copy link
Contributor

@gchoqueux gchoqueux commented Nov 24, 2025 •
edited
Loading

Description

  • Remove duplicated code related to three.js
  • Remove webpack externals from the worker to fix imports in three.js
  • Update three.js to use the new serialize methods
bundle Master this PR
itowns_lasworker.js 214KiB 240 KiB

@gchoqueux gchoqueux marked this pull request as draft November 24, 2025 16:20
@gchoqueux gchoqueux force-pushed the refacto/worker branch 2 times, most recently from 7d9f96a to 56a8cc9 Compare November 24, 2025 16:51
@gchoqueux gchoqueux marked this pull request as ready for review November 24, 2025 16:55
@Desplandis
Copy link
Contributor

Desplandis commented Dec 12, 2025

Would it be possible to bump three.js in a different PR?

@gchoqueux gchoqueux force-pushed the refacto/worker branch 3 times, most recently from fac11ea to 3c68c40 Compare December 19, 2025 15:52
@gchoqueux
Copy link
Contributor Author

gchoqueux commented Dec 19, 2025

Would it be possible to bump three.js in a different PR?

done #2663

ftoromanoff
ftoromanoff reviewed Jan 7, 2026
View reviewed changes
packages/Main/src/Loader/LASLoader.js
Comment on lines +4 to +10
// Don’t import Three directly if you want to improve tree shaking.
// eslint-disable-next-line import/extensions
import { Vector3 } from 'three/src/math/Vector3.js';
// eslint-disable-next-line import/extensions
import { Quaternion } from 'three/src/math/Quaternion.js';
// eslint-disable-next-line import/extensions
import { Box3 } from 'three/src/math/Box3.js';
Copy link
Contributor

@ftoromanoff ftoromanoff Jan 7, 2026

Choose a reason for hiding this comment

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

Is it really needed to do the import in 3 lignes?
I understand that it's better than to do
import * as THREE from three
but can't we do something like that instead ?
import { Vector3, Quaternion, Box3 } from three/src/math

Copy link
Contributor Author

@gchoqueux gchoqueux Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

yes, it' needed to import from specific files for the tree shacking

Copy link
Contributor

@ftoromanoff ftoromanoff Jan 7, 2026

Choose a reason for hiding this comment

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

Ok, thus that what should be done in all the other itowns files then ?

Copy link
Contributor

@Desplandis Desplandis Jan 8, 2026

Choose a reason for hiding this comment

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

Normally, import { Vector3, Quaternion, Box3 } from three shall be sufficient for webpack to do tree-shaking.

@gchoqueux
fix(worker): no support three in worker
14651b4 
* avoid to externalize three for worker
ftoromanoff
ftoromanoff reviewed Jan 7, 2026
View reviewed changes
packages/Main/src/Parser/LASParser.js Outdated
Comment on lines 26 to 49
const positionBuffer = new THREE.BufferAttribute(attributes.positions, 3);
geometry.setAttribute('position', positionBuffer);

const intensityBuffer = new THREE.BufferAttribute(attributes.intensity, 1);
const intensityBuffer = new THREE.BufferAttribute(attributes.Intensity, 1);
geometry.setAttribute('intensity', intensityBuffer);

const returnNumber = new THREE.BufferAttribute(attributes.returnNumber, 1);
const returnNumber = new THREE.BufferAttribute(attributes.ReturnNumber, 1);
geometry.setAttribute('returnNumber', returnNumber);

const numberOfReturns = new THREE.BufferAttribute(attributes.numberOfReturns, 1);
const numberOfReturns = new THREE.BufferAttribute(attributes.NumberOfReturns, 1);
geometry.setAttribute('numberOfReturns', numberOfReturns);

const classBuffer = new THREE.BufferAttribute(attributes.classification, 1);
const classBuffer = new THREE.BufferAttribute(attributes.Classification, 1);
geometry.setAttribute('classification', classBuffer);

const pointSourceID = new THREE.BufferAttribute(attributes.pointSourceID, 1);
const pointSourceID = new THREE.BufferAttribute(attributes.PointSourceId, 1);
geometry.setAttribute('pointSourceID', pointSourceID);

if (attributes.color) {
const colorBuffer = new THREE.BufferAttribute(attributes.color, 4, true);
if (attributes.colors) {
const colorBuffer = new THREE.BufferAttribute(attributes.colors, 4, true);
geometry.setAttribute('color', colorBuffer);
}
const scanAngle = new THREE.BufferAttribute(attributes.scanAngle, 1);
const scanAngle = new THREE.BufferAttribute(attributes.ScanAngle, 1);
geometry.setAttribute('scanAngle', scanAngle);
Copy link
Contributor

@ftoromanoff ftoromanoff Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Is it really a good idea to rename the attributes with a capital letter (I uinderstand it comes from the name used by copc library (https://github.com/connormanning/copc.js/blob/master/src/las/extractor.ts)
but then maybe we need to use Position instead of positions and Color instead of colors, or maybe better use a map to associate the name inplural with the name with Capital letter (Array LASAttributes in LasLoader.js)

Copy link
Contributor Author

@gchoqueux gchoqueux Jan 8, 2026

Choose a reason for hiding this comment

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

I refactored this part to improve maintainability and future evolution

Copy link
Contributor

@Desplandis Desplandis Jan 8, 2026

Choose a reason for hiding this comment

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

Could you explain the expected future gains?

Copy link
Contributor Author

@gchoqueux gchoqueux Jan 8, 2026

Choose a reason for hiding this comment

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

to add other attributes
https://github.com/connormanning/copc.js/blob/master/src/las/extractor.ts#L173

Desplandis
Desplandis reviewed Jan 8, 2026
View reviewed changes
webpack.config.cjs
Comment on lines +159 to +166
externals: [
({ context, request }, callback) => {
if (request === 'three' && !excludesToExternals.find(p => context.includes(p))) {
return callback(null, 'three');
}
callback();
},
],
Copy link
Contributor

@Desplandis Desplandis Jan 8, 2026

Choose a reason for hiding this comment

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

Yikes... This is pretty hacky but I don't see another way to do it.
This is in my opinion a reason why the ES bundle in webpack is experimental. We should change the bundler one day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ftoromanoff ftoromanoff ftoromanoff left review comments

@Desplandis Desplandis Desplandis left review comments

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

Assignees

No one assigned

Labels

chore 🚚 refactor 🏭

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gchoqueux @Desplandis @ftoromanoff