-
Notifications
You must be signed in to change notification settings - Fork 216
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
PointCloud2 and LaserScan #218
Conversation
cc50064
to
3ae072d
Compare
I added links in the kitti.html example so that people can get the dataset more easily. |
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've posted a few suggested changes
src/sensors/Particles.js
Outdated
var customUniforms = | ||
{ | ||
texture: { type: 't', value: THREE.ImageUtils.loadTexture( texture ) }, | ||
texture: { type: 't', value: new THREE.TextureLoader().load( texture ) } |
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.
Most of the ros3djs code uses spaces, so might be nice to remove the tab here
src/sensors/Particles.js
Outdated
}; | ||
|
||
this.attribs = | ||
|
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 extra spaces there are noise too. Some people get finicky about these things :-)
src/sensors/Particles.js
Outdated
var customUniforms = | ||
{ | ||
texture: { type: 't', value: THREE.ImageUtils.loadTexture( texture ) }, | ||
texture: { type: 't', value: new THREE.TextureLoader().load( texture ) } | ||
//texture: { type: 't', value: THREE.ImageUtils.loadTexture( texture ) }, |
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 you comment this code out, it might live there for ever
Git keeps the history, so just remove the line. People can refer to git if they want to know why and when it was remove
src/sensors/Particles.js
Outdated
this.attribs = | ||
|
||
|
||
/*this.attribs = |
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.
same here if commenting something out, just remove it
src/sensors/Particles.js
Outdated
var customColor = []; | ||
var alpha = []; | ||
|
||
for(var i = 0; i < this.max_pts; i++){ |
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 this perhaps be done with something like
positions = new Array(3 * this.max_pts).fill(0);
Likely to be significantly more efficient with the browsers array fill.
It's reasonably well supported. I'm not sure how far back this project expects browser support, but it's been around for a while
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/fill
src/sensors/Particles.js
Outdated
particles.attribs.customColor.needsUpdate = true; | ||
particles.attribs.alpha.needsUpdate = true; | ||
particles.points.needsUpdate = true; | ||
//particles.attribs.customColor.needsUpdate = true; |
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.
remove this and the like 145 that is commented out
oh I posted some suggestions, then realised you removed some of that code later anyway :-P oops |
Sorry about that, i can squash the 2 commits if you prefer. |
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.
Thanks for the PR. It looks good. Could you address the comments in the code?
And could you remove package-lock.json
?
* * texture - (optional) Image url for a texture to use for the points. Defaults to a single white pixel. | ||
* * rootObject (optional) - the root object to add this marker to | ||
* * size (optional) - size to draw each point (default 0.05) | ||
* * max_pts (optional) - number of points to draw (default 100) |
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.
color
, size
and texture
are removed. Could you check if these fields are set and log deprecation warning with a guide to use the newer way?
src/sensors/Particles.js
Outdated
* * texture - (optional) Image url for a texture to use for the points. Defaults to a single white pixel. | ||
* * rootObject (optional) - the root object to add this marker to | ||
* * size (optional) - size to draw each point (default 0.05) | ||
* * max_pts (optional) - number of points to draw (default 100) |
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.
texture
and size
are gone here also. Could you add a check with deprecation warning?
Deprecation warning as been added here. |
Removing redundant roslaunch calls in the help.
- This pulls in changes from PRs RobotWebTools#207, RobotWebTools#210, RobotWebTools#218, RobotWebTools#221, and RobotWebTools#223 - Note: src/sensors/Points.js was modified to *explicitly* extend THREE.Object3D to support transpiling
- This pulls in changes from PRs RobotWebTools#207, RobotWebTools#210, RobotWebTools#218, RobotWebTools#221, and RobotWebTools#223 - Note: src/sensors/Points.js was modified to *explicitly* extend THREE.Object3D to support transpiling
- This pulls in changes from PRs RobotWebTools#207, RobotWebTools#210, RobotWebTools#218, RobotWebTools#221, and RobotWebTools#223 - Note: src/sensors/Points.js was modified to *explicitly* extend THREE.Object3D to support transpiling
- This pulls in changes from PRs RobotWebTools#207, RobotWebTools#210, RobotWebTools#218, RobotWebTools#221, and RobotWebTools#223 - Note: src/sensors/Points.js was modified to *explicitly* extend THREE.Object3D to support transpiling
- This pulls in changes from PRs RobotWebTools#207, RobotWebTools#210, RobotWebTools#218, RobotWebTools#221, and RobotWebTools#223 - Note: src/sensors/Points.js was modified to *explicitly* extend THREE.Object3D to support transpiling
* Fix links in example and readme (#206) * Report error from ColladaLoader in MeshResource (#210) this will hopefully make situations like #209 easier to debug * Added Rollup * Added yarn lockfile * Removed references to COLLADA_LOADER_2 * Added es6 transpiler and rollup config, working on transpiling * Touched up a couple potential mistypes prevent transpiler from working. 1 - removed explicit super.super() call from InteractiveMarker. 2 - Made Particles explicitly derive from THREE.Object3D. * Rewrote Particles' method signatures in format consistent with rest of codebase * Refactored updateMatrixWorld to be more statically analyzable * ES6 modules properly compiling, Working on runtime errors * Moved 'that' assignments happen *after* calls super constructors in derived classes * Added missing super constructor call * Removed assignment to read-only property causing a runtime error, added a relevant comment * Moved all super constructor calls to preceed any use of 'this' * Added shims for THREE and THREE extensions to support es6 compatible module extensions * Cleanup and added examples for single page applications and using html imports in browsers * Moved es6 support files to es6-support folder and renamed destination for es6 transpiled output * Moved shims to top level * Removed pre-es6 source code and es6 transpiler * DepthCloud: make depth range adaptable + fix depth point position decoding (#207) * PointCloud2 and LaserScan (#218) * buffergeometry in PointCloud2 * pointcloud2: buffergeometry, subsampling * renamed Particles.js to Points.js, deprecation warning in Points.js * Update kitti.html * Removing redundant roslaunch calls in the help. * NavSatFix support (#221) * fix: pointRatio option in PointCloud2 and LaserScan (#223) * Added build products * Removed extra log * Added angular app example * Added react app example and touched up other SPA examples * Added build output * Removed extra log * Added angular app example * Added react app example and touched up other SPA examples * Updated ROSLIB import semantics * Updated ROSLIB import semantics * Switched ColladaLoader shim from official ColladaLoader to the ros3djs fork * Switched ColladaLoader shim from official ColladaLoader to the ros3djs fork * Updated html-import example to use yarn like the other examples * Updated html-import example to use yarn like the other examples * Updated outdated grunt plugins * Fixed linter errors * Updated grunt build process to build es6 output * Switched from jshint to eslint, migrating rules and fixing lint issues * Updated commonjs target to be es5 * Fixed pkg.module to use es6 module syntax, but es5 language features * Fixed bug in Points class * Switched from const to var for es5 compatibility * Added PointCloud2 example for angular app * Updated node version use by CI server
This PR builds on #216 and #217 to address #213 and give extra functionnalities to PointCloud2 and LaserScan topics :
Misc:
examples/ros3djs.launch
to simplify setup to simplyroslaunch examples/ros3djs.launch
instead of launching manuallyroscore
,rosbridge
andtf2_web_republisher
.kitti.html
example that is intended to be used to visualize rossified kitti datasets.