-
Notifications
You must be signed in to change notification settings - Fork 189
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
Kinetic/fix/vertical fov bug #266
Open
yShzZpp
wants to merge
12
commits into
SteveMacenski:kinetic-devel
Choose a base branch
from
yShzZpp:kinetic/fix/verticalFovBug
base: kinetic-devel
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
192445f
ADD: start and end vertocal_fov_angle
yShzZpp 4b4f575
ADD: README
yShzZpp b23bd98
ADD: fix bug
yShzZpp e5d8478
ADD: params
yShzZpp c127311
ADD: Change the default values of start and end
yShzZpp 220dc69
ADD: fix depth camera bug
yShzZpp c5fd3ea
ADD: fix MeasurementReading and layer bug
yShzZpp 257707c
ADD: use one contructor
yShzZpp 3b011f3
ADD: fix bug and optimazed code
yShzZpp 80741f9
ADD: fix formating errors
yShzZpp 83219ff
ADD: fix formating errors
yShzZpp 01f33d9
ADD: fix formating errors
yShzZpp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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.
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.
Please make sure to test, in detail, this logic, since we've found now 2 different errors in it that weren't caught before. test the boundaries of the different conditions and make sure this finally now works properly at the correct limit conditions
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'm really sorry. I'm a beginner, and I've tried my best to locate where the problem might be, but with no success so far. I'm not sure if this will inconvenience you, but I still hope you could lend me a hand and point out where the issue lies. I'll do my best to fix it.
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 don't know if there is an issue - I'm simply asking for you to write some tests to make sure this is correct in dealing with your different start/end FOV angles. If its not working, then look back at the geometry :-)
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'm not sure how to write tests, but I can describe our application scenario to you.
Our robot is a cuboid (1.000.50.3) with a front end that has a height of 0.7cm. Previously, we used 2D lidars, which only provided information about a single plane at a height of 0.2 meters. Now, due to new requirements, the robot needs to perceive obstacles below a height of 3 meters in front. Because the robot design is fixed, a 3D lidar can only be installed in the front part of the robot, at a height of 0.47 meters.
The lidar we are using is from Livox, with a FOV of -7° to 53°. Assuming our mounting angle is a roll angle of 90°, the angles it can see are illustrated in the diagram below.
Therefore, in the existing code, if I set the vertical FOV (vfov) to 114 degrees, points with positive values on the z-axis in the lidar's coordinate system will be correctly cleared. However, points with negative z-values or those below -7° will be cleared as well, which is not the desired outcome. On the other hand, if the vfov is set to a value less than 114 degrees, there will be residual points on the positive z-axis that cannot be cleared.
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 can appreciate that - so what's the problem? What aren't you able to test about your own code contribution to make sure it actually does what you want it to do? Previous iterations of this PR clearly didn't work properly so I can tell you didn't test in detail to make sure the behavior you want as working. What are you going to test now with the changes to ensure your changes work as expected?