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

Adaptable depthcloud range #207

Conversation

viktorku
Copy link
Member

@viktorku viktorku commented Jan 9, 2018

The DepthCloud implementation that goes along with RobotWebTools/depthcloud_encoder#8.

Also fixes the decoding step as the depthcloud hasn't been adapted since RobotWebTools/depthcloud_encoder#5 has been merged to correctly accommodate for the fixed range.

Copy link
Member

@jihoonl jihoonl left a comment

Choose a reason for hiding this comment

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

It looks like it changes the default behavior with default value of maxDepthPerTile. Can you make the default behavior as before?

' ( position.x / width - 0.5 ) * z * (1000.0/focallength) * -1.0,',
' ( position.y / height - 0.5 ) * z * (1000.0/focallength),',
' (- z + zOffset / 1000.0) * 2.0,',
' ( position.x / width - 0.5 ) * z * 0.5 * maxDepthPerTile * (1000.0/focallength) * -1.0,',
Copy link
Member

Choose a reason for hiding this comment

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

It looks like it changes the default behavior with default value of maxDepthPerTile. Can you make the default behavior as before?

Copy link
Member Author

Choose a reason for hiding this comment

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

As I mentioned in the PR description, when RobotWebTools/depthcloud_encoder#5 was merged, this file hasn't been updated for the decoded depth cloud to accommodate for the reduced factor. This was actually a bug that places all points at twice the distance from their correct pose, because we are encoding at a singular/unit range (1.0 = 0 - 2m) and decoding at twice the range (2.0 = ~4m). Hence this PR halves the distance in each axis.

Copy link
Member Author

Choose a reason for hiding this comment

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

There actually was a PR that never made it in, because of too many untested changes. The change in question is here: https://github.com/RobotWebTools/ros3djs/pull/106/files#diff-32279e45de1135eb1be4b5f7120f695aL156

Copy link
Member

Choose a reason for hiding this comment

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

@viktorku oh I see, any thoughts on a more general solution? For instance do you think it may be possible to provide the min/max range values directly in the data from the depthcloud_encoder so that the resolution can be dynamically and automatically configured on the receiving side here in ros3djs? It seems our current approach is a pretty poor user experience to have to tune both sides manually, maybe we should go after a more general improvement after we get this bug fix through.

I'll give this a spin on my equipment this evening.

Copy link
Member

@jihoonl jihoonl Jan 12, 2018

Choose a reason for hiding this comment

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

@viktorku I see. I missed to read your comments. thanks for the explanation.
@sevenbitbyte maybe user can rosparam get max_depth_per_tile before creating DepthCloud object to get the parameter from server side. How about adding this step in our depthcloud example?

Copy link
Member

Choose a reason for hiding this comment

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

@sevenbitbyte any updates? Otherwise, we move on.

@jihoonl
Copy link
Member

jihoonl commented Jan 18, 2018

@viktorku Could you update the depth_cloud example to get max_depth_per_tile param from robot? then feel free to merge this in.

@jihoonl
Copy link
Member

jihoonl commented Apr 8, 2018

Merged via 560ca2e

@jihoonl jihoonl closed this Apr 8, 2018
keego added a commit to keego/ros3djs that referenced this pull request Apr 29, 2018
- 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
keego added a commit to keego/ros3djs that referenced this pull request Apr 29, 2018
- 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
keego added a commit to keego/ros3djs that referenced this pull request Apr 30, 2018
- 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
keego added a commit to keego/ros3djs that referenced this pull request Apr 30, 2018
- 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
keego added a commit to keego/ros3djs that referenced this pull request May 1, 2018
- 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
jihoonl pushed a commit that referenced this pull request Aug 24, 2018
* 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
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