rosbag2_cpp: sequential reader specifies no order

[james-rms]

Right now SequentialReader chooses a default read order (by default constructing ReadOrder) and calls set_read_order() on the underlying storage plugin. However, the underlying storage plugin may not support that read order. If the user hasn't specified the read order they want, the plugin should just decide for them.

It doesn't really make sense for SequentialReader to always specify the read order, unless the user has actually called set_read_order on SequentialReader. With that in mind this PR removes that constraint.

Fixes ros-tooling/rosbag2_storage_mcap#61

[james-rms]

@emersonknapp are you able to review? (I don't have permission to specifically assign you)

[emersonknapp]

I think that this approach makes sense - there's no particular reason for the SequentialReader to have an opinion unless the user explicitly asks for it. The plugin can have its own default read order.

[emersonknapp]

Does SequentialReader::set_read_order need any update? Or does the = operator work ok for optional. It's not something I've used much yet

[emersonknapp]

LGTM with green CI

[MichaelOrlov]

I have a concern about this PR.
I agree that storage plugin could have some default read order.
However in SequantialReader we have some logic which is rely on read order and falling back to some assumption what default read order would be for the plugin without explicitly reading it out also incorrect behavior!
For instance in changes from this PR made implicit assumption for SequantialReader that default read order for storage plugin is not reverse which might not be necessary the case.

I also opposed to use optional.
While it looks handy and and fancy new C++ feature - it's actually an "evil" for many corner cases and introduce more uncertainty in design and also require more boiler plate dummy checks for nullopt which is very easy to miss or incorrectly mixup with other existent logic.

The right solution if we really want to respect default read order from storage plugin would be to bring in a new API aka rosbag2_storage::ReadOrder rosbag2_storage::get_read_order() and use it in constructor for initialization of the private read_roder variable in SequantialReader.

I am honestly very doubt that this is a big deal or even a meaningful issue that we are falling back to some predefined read order in SequantialReader and enforcing it for underlying storage plugin if it was not set explicitly.
Thea are not too many storage plugins to be worry about this default value.
IMO current behavior is a WAD (worked as designed) and ok to be as is.
Storage plugin shall raise exception if it doesn't support read order settling up from SequantialReader.

[jhdcs]

I am curious why optional is "evil". Isn't is supposed to be used to prevent issues with checking for null? How is that evil?

[james-rms]

Going with a different approach.

[MichaelOrlov]

@jhdcs

I am curious why optional is "evil". Isn't is supposed to be used to prevent issues with checking for null? How is that evil?

Here is the real life example while it is "Evil" ros-tooling/topic_tools#48
C++ optional introduces undefined behavior or at least in some cases trying to handle this undefined behavior.
Usually using optional means that present "sub-optimal" design with undefined behavior. In general need to avoid such solutions and designs as much as possible.
C++ optional is intend to be used when there are not possible to change design to avoid such undefined behavior - in all other cases usage of the optional as handy snippet to handle logic shall be avoided as much as possible.