RSDK-2821 Update Frontend SLAM 2D View to XY Plane #2269
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
the
safe to test
viambot
added
safe to test
viambot
added
safe to test
jeremyrhyde
changed the title
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
jeremyrhyde
commented
Apr 25, 2023
| @@ -72,7 +72,7 @@ raycaster.on('click', (event) => { | |||
|
|
|||
| const markerSize = 0.5; | |||
| const marker = new THREE.Mesh( | |||
| new THREE.PlaneGeometry(markerSize, markerSize).rotateX(-Math.PI / 2), | |||
There was a problem hiding this comment.
[note] default plane is XY so a rotation here is no longer necessary
jeremyrhyde
commented
Apr 25, 2023
| @@ -50843,6 +50843,230 @@ | |||
| } | |||
| }, | |||
| "position_new": {} | |||
| }, | |||
| "viam-office-02-22-2": { | |||
There was a problem hiding this comment.
[note] I've left the old artifacts in for the present. I am planning to remove them after we've stabilized the frontend view but can delete them now if desired. Comment here if you'd like me to remove them now.
There was a problem hiding this comment.
I'm trusting that this dataset will be regenerated from the final version of viam-modules/viam-cartographer#59 once that viam-cartographer PR is is merged, before this PR is merged.
…ateSLAM2DViewToXY
viambot
added
safe to test
viambot
added
safe to test
jeremyrhyde
requested review from
nicksanford,
raybjork,
EshaMaharishi and
biotinker
and removed request for
raybjork
biotinker
reviewed
Apr 25, 2023
services/slam/fake/slam_test.go
Outdated
| r3.Vector{X: -0.005666600181385561, Y: -6.933830159344678e-10, Z: -0.013030459250151614}, | ||
| &spatialmath.Quaternion{Real: 0.9999999087728241, Imag: 0, Jmag: 0.0005374749356603168, Kmag: 0}) | ||
| r3.Vector{X: -0.005839621552532072747133, Y: 0.012640521020122929413132, Z: 0.000000000000000000}, | ||
| &spatialmath.Quaternion{Real: 0.9999998930633826, Imag: 0, Jmag: 0, Kmag: -0.000462464294427742}) |
There was a problem hiding this comment.
Is this definitely the actual orientation you want? It is sooooo close to (1,0,0,0) in which case you could just use NewPoseFromPoint and leave off the quaternion entirely. My guess is that PoseAlmostEqual will still be true
There was a problem hiding this comment.
I'd prefer to keep the explicit definitions because the test is loading the first pointcloud and in future datasets this first pointcloud may include some small degree of rotation and this will be easier to update with new datasets if we leave these written out in longhand.
@nicksanford @EshaMaharishi I'd like to hear your thoughts on this.
Peter is correct we can use the default orientation here. Although, we can't use the full: spatialmath.NewZeroPose() as the PoseAlmostEqual function still fails when comparing the r3.vector from the first fake position dataset defined above with (0,0,0).
There was a problem hiding this comment.
NewPoseFromPoint is a convenience function that will accept your r3.Vector and fill in the zero orientation automatically
There was a problem hiding this comment.
IMO this test should break if we change the test dataset. I think it should reflect the dataset's actual value, i.e. what you already have @jeremyrhyde.
There was a problem hiding this comment.
To be clear; NewPoseFromPoint doesn't ignore orientation, it simply auto-fills (1,0,0,0), which is functionally the same as what's currently used. So this would still break if the tests dataset does.
Totally optional change though.
nicksanford
requested changes
Apr 25, 2023
biotinker
reviewed
Apr 25, 2023
viambot
added
safe to test
nicksanford
approved these changes
Apr 26, 2023
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
|
bazile-clyde
pushed a commit
to bazile-clyde/rdk
that referenced
this pull request
Jan 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the 2D projection view of the SLAM map on the frontend to view the new plane being used by the SLAM map (XY). In addition, this PR includes an updated list of artifacts with data in this new plane for fake slam to use.
New artifacts will be in a folder:
viam-office-02-22-2Old artifacts will continue to exist in their current folder:
viam-office-02-22-1Screen.Recording.2023-04-25.at.5.13.48.PM.mov
Note: There are some minor issues in this video and therefore the fake slam dataset that includes small areas of bad mapping, and jumping. I can attempt to regenerate it to see if that fixes it but as this dataset will be replaced with in the next week or so once the map-flipping ticket is resolved, I didn't find it necessary to clean up the dataset.
JIRA Tickets:
Associated PR: