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

PointCloudLayer: use view.crs in LasParser (as the other parsers) #2498

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ftoromanoff
Copy link
Contributor

@ftoromanoff ftoromanoff commented Feb 10, 2025

The aim of the PR is to allow projection of dataPointCloud in a view crs different than the source crs.
For the projection we use proj4.

There is two main parts in this PR:

  • the projection of the bounding box read from metadonne in the layer.
  • the projection of the data during the parsing.

Instead of doing pure projection we add after the projection, an transformation of data in a local system orienté in the direction of the data and z orthogonal.

Other minor change:

  • the 'spacing' is move from source to the layer.
  • A 'elevation' propertie was also added to replace the z data (that is not an elevation)
  • update examples

@ftoromanoff ftoromanoff changed the title PointCloudLayer: use view.crs in LasParser (as for other parser) PointCloudLayer: use view.crs in LasParser (as the other parsers) Feb 10, 2025
@ftoromanoff ftoromanoff marked this pull request as ready for review February 10, 2025 13:34
Comment on lines 65 to 57
const bounds = [
...forward(cube.slice(0, 3)),
...forward(cube.slice(3, 6)),
];

this.root.bbox.setFromArray(bounds);

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be factorized in parent method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see previous reply.

@ftoromanoff ftoromanoff force-pushed the ptCloudReproj branch 2 times, most recently from 80bafd6 to 4b75a24 Compare February 12, 2025 13:25
Comment on lines +53 to +55
const forward = (options.in.crs !== options.out.crs) ?
proj4(options.in.projDefs, options.out.projDefs).forward :
(x => x);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is same part with foward, could you factorize ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LASLoader will be called by the worker, so we need to limite the dependances, thus it would be counter-productive.

positions[i * 3] = x - origin[0];
positions[i * 3 + 1] = y - origin[1];
positions[i * 3 + 2] = z - origin[2];
Copy link
Contributor

@gchoqueux gchoqueux Feb 12, 2025

Choose a reason for hiding this comment

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

it's fist transformation to move on local space
There isn't the rotation

Comment on lines 115 to 138
in: {
crs: options.in.crs,
projDefs: proj4.defs(options.in.crs),
},
out: {
crs: options.out.crs,
projDefs: proj4.defs(options.out.crs),
},
Copy link
Contributor

@gchoqueux gchoqueux Feb 12, 2025

Choose a reason for hiding this comment

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

Factorize with the line 154

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really see the benefit of doing so, moreover when we have the idea (in the futur) to allows proj4.defs as crs. In this cas it will simplifed the options as passing only the crs.

@@ -231,6 +232,29 @@ class PointCloudLayer extends GeometryLayer {
this.root = undefined;
}

setRootBbox(min, max) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you the same signature and this
setRootBboxFromArray(bounds)

Copy link
Contributor Author

@ftoromanoff ftoromanoff Feb 13, 2025

Choose a reason for hiding this comment

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

If i understand you, you want to add another methode, with a different signature. But currently this function is call 4 times, if we add a 2nd one, both will be called 2 times, won't it be unfactorizing some code ?

@ftoromanoff ftoromanoff force-pushed the ptCloudReproj branch 9 times, most recently from 60e9d9a to b4a32ce Compare February 27, 2025 13:23
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