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

Offline SLAM Small Bug Fix #5167

Merged
merged 3 commits into from
Jun 9, 2022

Conversation

yuecideng
Copy link
Collaborator

@yuecideng yuecideng commented Jun 8, 2022

  1. Change depth_max -> max_depth.
  2. Add --vis to control whether to visualize the result.

This change is Reviewable

@update-docs
Copy link

update-docs bot commented Jun 8, 2022

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@yxlao yxlao self-requested a review June 9, 2022 02:17
Copy link
Collaborator

@yxlao yxlao left a comment

Choose a reason for hiding this comment

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

image
image

There are some inconsistencies across the library regarding whether to use depth_max or max_depth. A quick search shows that depth_max is more often used than max_depth. I think depth_max is preferred, as it is consistent with depth_scale.

Could you do a search-replace to convert all max_depth to depth_max for everything related to the reconstruction system? For other use cases, e.g. the octree, we don't need to change.

@yuecideng
Copy link
Collaborator Author

Could you do a search-replace to convert all max_depth to depth_max for everything related to the reconstruction system? For other use cases, e.g. the octree, we don't need to change

No problem, but should we change them in OdometryOption? Because I notice that there are both max_depth, min_depth and max_depth_diff. Should we also change min_depth -> depth_min and max_depth_diff -> depth_diff_min?

@yxlao
Copy link
Collaborator

yxlao commented Jun 9, 2022

No problem, but should we change them in OdometryOption? Because I notice that there are both max_depth, min_depth and max_depth_diff. Should we also change min_depth -> depth_min and max_depth_diff -> depth_diff_min?

Yes, that sounds good to me.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@yxlao yxlao self-requested a review June 9, 2022 11:06
Copy link
Collaborator

@yxlao yxlao left a comment

Choose a reason for hiding this comment

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

LGTM after CI.

@yxlao yxlao merged commit c8f8f40 into isl-org:master Jun 9, 2022
@yuecideng yuecideng deleted the yuecideng/OfflineSLAMSmallBugFix branch June 10, 2022 03:28
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