search

zivid / zivid-ros

Official ROS driver for Zivid 3D cameras
BSD 3-Clause "New" or "Revised" License
55 stars 43 forks source link

Add support for loading settings from file #60

Closed apartridge closed 2 years ago

apartridge commented 2 years ago

Fixes issue #33.

Was based on PR https://github.com/zivid/zivid-ros/pull/59 from @danielcranston

apartridge commented 2 years ago

Thanks for getting to this so quickly.

Getting rid of the rosparam for specifying a settings file at startup (and relying only on the service) sounds good to me, and that recursivelyFillInUnsetWithCameraDefault machinery really helps with user experience +1

One potentially surprising behavior is that if you load_settings_from_file, specifying a file that omits some setting, that setting will be force-set to the default value (due to FillInUnsetWithCameraDefault) even if that setting has previously been set.

Maybe a more intuitive behavior would be for settings that are not explicitly mentioned in the incoming setting file to be left at their current value, not be hard-set back to the default value? Essentially, FillInUnsetWithCurrentValues. I don't have any strong opinions here though, just thought I'd bring it up.

The behavior we have in the main C++/C#/Python API is that if a setting value is not provided, then it reverts back to camera model's default value (and not what was used for the last capture, for instance). So I think we want to follow the same behavior for the ROS driver. For example, it is preferable that if you load a.yml in Zivid Studio or in zivid-ros and capture then you always get the same output, independent of previous state of the ROS driver.

runenordmo commented 2 years ago

No more comments from me, looks good :+1:

apartridge commented 2 years ago

The review got dismissed so you need to re-approve it @runenordmo