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

Improve spatial_hash function to avoid spatial periodicity in the hash function #273

Merged
merged 2 commits into from
Dec 28, 2023

Conversation

glpuga
Copy link
Collaborator

@glpuga glpuga commented Dec 25, 2023

Proposed changes

Adds two commits:

  • The first one adds periodicity checks that the existing hash function does not pass.
  • The second one updates the hash function to pass old and new tests.

Based on:

Type of change

  • 🐛 Bugfix (change which fixes an issue)
  • 🚀 Feature (change which adds functionality)
  • 📚 Documentation (change which fixes or extends documentation)

💥 Breaking change! Explain why a non-backwards compatible change is necessary or remove this line entirely if not applicable.

Checklist

Put an x in the boxes that apply. This is simply a reminder of what we will require before merging your code.

  • Lint and unit tests (if any) pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All commits have been signed for DCO

Additional comments

Anything worth mentioning to the reviewers.

@glpuga glpuga force-pushed the glpuga/spatial_hash_robustness branch from fe25bd3 to 72cb90e Compare December 25, 2023 20:27
@glpuga glpuga marked this pull request as ready for review December 25, 2023 20:28
@glpuga glpuga force-pushed the glpuga/spatial_hash_robustness branch 2 times, most recently from a00af18 to f86206d Compare December 25, 2023 21:10
Copy link
Member

@nahueespinosa nahueespinosa left a comment

Choose a reason for hiding this comment

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

@glpuga This is super interesting! What's the motivation behind this? Is this an improvement to better support 3D (6-DOF) or is it something else?

beluga/include/beluga/algorithm/spatial_hash.hpp Outdated Show resolved Hide resolved
@glpuga
Copy link
Collaborator Author

glpuga commented Dec 28, 2023

What's the motivation behind this? Is this an improvement to better support 3D (6-DOF) or is it something else?

This is related to #275 , since I'm reusing this for that estimator. While I don't need more than three dimensions there, I noticed the periodicity in the hash when looking at the code. I don't think we should ignore the information in the upper bits of each dimension, it's better to blend it all together by spreading information in all the space.

Signed-off-by: Gerardo Puga <glpuga@ekumenlabs.com>
Signed-off-by: Gerardo Puga <glpuga@gmail.com>
Signed-off-by: Gerardo Puga <glpuga@ekumenlabs.com>
Signed-off-by: Gerardo Puga <glpuga@gmail.com>
@glpuga glpuga force-pushed the glpuga/spatial_hash_robustness branch from bf7e723 to d616636 Compare December 28, 2023 15:49
Copy link
Member

@nahueespinosa nahueespinosa left a comment

Choose a reason for hiding this comment

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

LGTM!

@glpuga glpuga merged commit 4f080fe into main Dec 28, 2023
8 checks passed
@glpuga glpuga deleted the glpuga/spatial_hash_robustness branch December 28, 2023 17:23
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.

None yet

2 participants