scan_offset parameter for Ros2

[YBachmann]

Similar to this PR: #28 I added a scan_offset parameter for the ROS2 version.

[YBachmann]

@chadrockey I just wanted to aks if you could make a statement on roughly when someone could look at this PR?

[clalancette]

@YBachmann This is failing all of the tests because it doesn't conform to our style. Please fix the tests to pass, then we can take a look at reviewing it.

[YBachmann]

@YBachmann This is failing all of the tests because it doesn't conform to our style. Please fix the tests to pass, then we can take a look at reviewing it.

@clalancette The tests are now passing, sorry for not having them fixed earlier.

[clalancette]

My major question here has to do with whether scan_offset makes sense as a float. Since 0.0 means the top, and 1.0 means the bottom, 0.5 means "in the middle". But what if there are an odd number of scan lines? The user would have no ability to choose exactly which line to use, and instead would just have to take whichever one 0.5 rounded down to.

That said, I'm not all too familiar with this stuff. But would it make sense to let the user pick a specific scan line?

[YBachmann]

I understand your concern, however in my opinion a float makes a lot of sense because:

• even if there are an odd number of lines (which is quite uncommon) you can still pick a specific line. The scan_offset value would is specific_line / image_height.
• I don't think it ist even necessary to specify an exact row for the vast majority of usecases. Most people will just roughly look at their depthimages and say "I want to use the top half of my images for the laserscan, so let's set scan-height to 0.25".
• As a user I would much rather have the ability to specify that I always want e.g. the center of the image to be used (scan_offset=0.5), rather than specify a specific row. This way the center will still be used even if I use a different image resolution in the future (e.g. because I scale down my images for performence reasons).

[YBachmann]

@clalancette do you have any open points you want to discuss or have changed?

PS: I also opened another PR #82 , I would really appreciate if you could take a look at it as well :)

[YBachmann]

Hello @clalancette ,
as some time went by, I just wanted to ping this to check if you want me to make some more changes or if this could be merged soon.

[YBachmann]

Hello @chadrockey ,

could you please take a look at this PR?
We had a discussion in PR #82 on whether it makes sense to add more functionality/complexity to this package, but I think this PR adds a nice and simple feature without much complexity.

PS: This PR also clarifies the description of scan_height in the README.md, which would make PR #79 irrelevant.

[YBachmann]

@chadrockey ?

[YBachmann]

@clalancette @chadrockey are there any plans to merge this still?

[trupples]

+1, this is a feature I needed, unknowingly (re)implemented almost identically, and which I intended to submit as well.