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

Support custom resolutions properly #242

Conversation

viktorku
Copy link
Member

@viktorku viktorku commented Dec 4, 2018

Adds/computes a resolutionFactor from the resolution of the encoded depthcloud image that needs to be applied for the pose of each point in the vertex shader. It only needs to be applied in X and Y, otherwise the pointcloud appears resolutionFactor smaller.

Discovered when using a custom resolution/cropsize with RobotWebTools/depthcloud_encoder#11

@viktorku viktorku requested a review from jihoonl December 4, 2018 14:34
Copy link
Contributor

@mvollrath mvollrath left a comment

Choose a reason for hiding this comment

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

Should the build/ changes be in this PR?

@@ -29,11 +29,10 @@ ROS3D.DepthCloud = function(options) {
this.pointSize = options.pointSize || 3;
this.width = options.width || 1024;
this.height = options.height || 1024;
this.resolutionFactor = Math.max(this.width, this.height) / 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there something special about 1024 besides being the default height/width? Should this be max / min instead of max / 1024?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be max / min instead of max / 1024?

No. It should be a ratio of the default 1024 resolution. The max is just for completeness, we can use either the width or height, since the encoded image is always square.

@viktorku
Copy link
Member Author

Should the build/ changes be in this PR?

It's good to update the build files for CDN purposes every once in a while. As long as that's in a separate commit I don't see an issue with it.

@mvollrath
Copy link
Contributor

The built files in this PR don't load due to mrdoob/three.js#12719

@viktorku viktorku force-pushed the support-custom-resolutions-properly branch from a6bdfdf to 27dbf62 Compare January 29, 2019 20:24
@viktorku
Copy link
Member Author

I removed the build. I'll merge this in as it doesn't break anything. Confirmed it's working fine.

@viktorku viktorku merged commit e71ff9d into RobotWebTools:develop Jan 29, 2019
@viktorku viktorku deleted the support-custom-resolutions-properly branch January 29, 2019 20:29
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