(feat SensorModelling) add pixel_order field

[RIFJo]

Signed-off-by: RIFJo jochmann@rt.rif-ev.de

#### Reference to a related issue in the repository
refers to #620

Add a description

Adds a "pixel_order" field and enum to allow mirrored versions of the pixel layouts. This is the result of the discussions with the participants in the sensor modelling workgroup meetings.

This pull request also includes the changes from Thomas Sedlmayer in #684

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

• My suggestion follows the style and contributors guidelines.
• I have taken care about the message documentation and the fields and enums documentation.
• I have done the DCO signoff.
• My changes generate no errors when passing CI tests.
• I have successfully implemented and tested my fix/feature locally.
• Appropriate reviewer(s) are assigned.

If you can’t check all of them, please explain why.
If all boxes are checked or commented and you have achieved at least one positive review, you can assign the label ReadyForCCBReview!

[RIFJo]

plus one more commit to clean up the triple-slash.

[LukasElster]

Outcome Sensor Modeling Group meeting 25.01.2023: Place PIXEL_ORDER_DEFAULT on position 0 and remove PIXEL_ORDER_UNKNOWN

[jdsika]

The unknown is a mandatory field in OSI due to the fact that it is an enum.

[jdsika]

we would have to change the rules for enum definitions then?

[thomassedlmayer]

We (Pierre, Lukas and I) were aware of that guideline in OSI but discussed that it would not make sense to ever use unknown in this context as this is just a "technical" enum describing the pixel order that applies to the image data. It is not some kind of classification for which the guideline makes sense. I think there already were some other places in OSI where it was decided that the unknown/other guideline does not have to be enforced, especially when describing technical stuff.

[jdsika]

We have to check if we can edit the interface_conventions.

My take here:

1. we add this feature to 3.6 and comply with the current enum convention and leave the values UNKNOWN and OTHER
2. we create an issue for 4.0 and rework the interface convention regarding ENUMS

[jdsika]

• Rename default to the actual pixel order
• Reference default enum value correctly in the comments

[ClemensLinnhoff]

• Rename default to the actual pixel order
• Reference default enum value correctly in the comments

I changed it accordingly. I also changed the (for me) confusing naming of "mirrored" to the actual pixel order.

@RIFJo does this work for you?

[RIFJo]

I also changed the (for me) confusing naming of "mirrored" to the actual pixel order.
@RIFJo does this work for you?

I am unhappy about this change of wording, but i can live with it. Maybe keep the enum entries as it is now, but mention the mirroring direction in the documentation comment ("the pixels are ordered right to left, top to bottom. This represents a mirroring about the vertical axis (left-to-right) compared to the default order") ?

Anyway, thats just minor nitpicking. I'm okay if this goes into 3.6 as it is now.

[thomassedlmayer]

Actually, we did not want every possible pixel order to be present in this enum but only the ones that manipulate the image in a way that can not be done by repositioning the sensor with the sensor mounting position (e.g. turn sensor by 180 deg upside down). So we only provide the fields for mirroring and otherwise the default should be used. I think this information should still somehow be provided in the description otherwise people will probably think that some pixel orders are just missing. Another option may be to not call the field "pixel order" but mirroring state or similar and also state that this field influences the image data's pixel order? It should be clear what we do/do not want to achieve with this field.

[jdsika]

Actually, we did not want every possible pixel order to be present in this enum but only the ones that manipulate the image in a way that can not be done by repositioning the sensor with the sensor mounting position (e.g. turn sensor by 180 deg upside down). So we only provide the fields for mirroring and otherwise the default should be used. I think this information should still somehow be provided in the description otherwise people will probably think that some pixel orders are just missing. Another option may be to not call the field "pixel order" but mirroring state or similar and also state that this field influences the image data's pixel order? It should be clear what we do/do not want to achieve with this field.

Feel free to add to the comments. The default is now clearly defined and IMO it is way easier to understand now. Setting an enum to standard or to left to right is telling the user in a better way what the standard is.

[ClemensLinnhoff]

Actually, we did not want every possible pixel order to be present in this enum but only the ones that manipulate the image in a way that can not be done by repositioning the sensor with the sensor mounting position (e.g. turn sensor by 180 deg upside down). So we only provide the fields for mirroring and otherwise the default should be used. I think this information should still somehow be provided in the description otherwise people will probably think that some pixel orders are just missing. Another option may be to not call the field "pixel order" but mirroring state or similar and also state that this field influences the image data's pixel order? It should be clear what we do/do not want to achieve with this field.

This is still part of the documentation:

// \note this enum does not contain an entry for right-to-left and bottom-to-top, because this is equivalent to a
// 180-degree rotation of the default, which should be indicated in the sensor coordinate
// system.
//

[pmai]

I'm unclear on the sudden changes to this PR; AFAICT the definitions in this PR were the result of various long running discussions in the sensor modelling WG. The now added changes do not seem to improve upon this, but rather seem to diverge, without fixing the enum issue (the default enum value should be 0).

So if this really is supposed to be rediscussed, it needs rediscussion in the sensor modeling group prior to being ready for CCB.

On the enum issue: The interface guidelines are just internal project guidelines and do not themselves have normative force; for ground-truth-like data this approach still makes sense, however for technical protocol-level configuration data, it does not, hence deviation is warranted, and has already happened, see the ReferenceLine Type enum, as an example.

Here the 0 value should be used for the old default behavior, since this keeps 3.5 and 3.6 interoperability consistent. This aids backward/forward-compatibility,

[pmai]

CCB on 2023-03-27: Merge as-is.