-
Notifications
You must be signed in to change notification settings - Fork 215
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
Export OcTree #417
Export OcTree #417
Conversation
@psaripp Could please look into why this doesn't build. You are the original author of this code if I am correct. |
b9f21bc
to
d030a0e
Compare
More robust to whitespace; fix dot bug (dots are any character in regex); Arguments are optional to constructors
Rename as to similar to OccupancyGrid*; Including seperation of all classes to their own file, needed by transpiler; Remove all seperate class property definitions; not supported by transpiler
@jihoonl Please be thorough on your review. I am not that sure about my work this time. |
I agree with renaming to OccupancyMap to OcTree. It looked all good to me with a quick look. 😉 @psaripp there are Octree module updates introduced in this PR. Would you double-check if it actually changes behavior of your code? |
@jihoonl especially the transpiler and the exports. I am not sure I should export more or less. I don't think it is needed to export base classes that are only used inside other classes. |
Looks like you fixed the bug in es6-transpiler that I encountered in #438. I deleted es6-transpiler in that PR, it's not worth maintaining that. |
@trusktr I will merge this one. Can you update your PR(s) if needed? |
@@ -407,13 +434,13 @@ ROS3D.OcTreeBase.prototype._buildFaces = function () { | |||
_insertFace: function (face, pos, size, color) { | |||
const indexCount = this.vertices.length / 3; | |||
|
|||
for (let vertex of face.vertices) { | |||
face.vertices.forEach(function(vertex) { |
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.
Why change all the for-of loops to forEach?
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 think it was producing linting warnings/errors.
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.
Fixed in the other PR
* develop: Update Build Export OcTree (RobotWebTools#417)
* develop: Update Build Export OcTree (RobotWebTools#417)
Bumps [karma](https://github.com/karma-runner/karma) from 3.1.4 to 6.3.2. - [Release notes](https://github.com/karma-runner/karma/releases) - [Changelog](https://github.com/karma-runner/karma/blob/master/CHANGELOG.md) - [Commits](karma-runner/karma@v3.1.4...v6.3.2) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
No description provided.