-
Notifications
You must be signed in to change notification settings - Fork 303
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
base: master
Are you sure you want to change the base?
Conversation
bd215e4
to
4508206
Compare
src/Layer/CopcLayer.js
Outdated
const bounds = [ | ||
...forward(cube.slice(0, 3)), | ||
...forward(cube.slice(3, 6)), | ||
]; | ||
|
||
this.root.bbox.setFromArray(bounds); | ||
|
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.
Could be factorized in parent method
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.
see previous reply.
80bafd6
to
4b75a24
Compare
const forward = (options.in.crs !== options.out.crs) ? | ||
proj4(options.in.projDefs, options.out.projDefs).forward : | ||
(x => x); |
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 is same part with foward, could you factorize ?
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.
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]; |
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.
it's fist transformation to move on local space
There isn't the rotation
src/Parser/LASParser.js
Outdated
in: { | ||
crs: options.in.crs, | ||
projDefs: proj4.defs(options.in.crs), | ||
}, | ||
out: { | ||
crs: options.out.crs, | ||
projDefs: proj4.defs(options.out.crs), | ||
}, |
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.
Factorize with the line 154
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.
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) { |
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.
could you the same signature and this
setRootBboxFromArray(bounds)
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.
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 ?
4b75a24
to
a564b6e
Compare
a564b6e
to
d09e0da
Compare
We use proj4 in LasParser, to projet data in the view.crs during parsing.
A 'elevation' propertie was also added to replace the z data (that is not an elevation)Other minor change: