Skip to content

Resolve "fix(core): object registration is overriden by XML configuration"

Description

Previously, there was no data prerequisite in a service. One can declare any data in the XML configuration of a service, and use it at runtime with getters. So basically you could configure a service with any data. It was even possible to change the access type dynamically.

Sight 21.0 enforced a prior declaration of data with sight::data::ptr. They allow specifying the key name, access type, as well as the auto-connect and the optional attributes. Normally, only declared data this way should be used in services.

However, for back compatibility purposes (especially for the very specific SConfigController and SConfigLauncher services), it is still possible to register data just from the XML configuration. We discovered that this is actually buggy and the XML configuration overrides the sight::data::ptr declaration. So it is for instance possible to declare input data and finally configure it as an inout. We also realized that the default auto-connect and optional attributes values were not respected with an XML configuration. The actual default values were those of the XML parser.

This merge request first fixes the override of the data declaration with the XML configuration. It also fixes the XML parser to use the default auto-connect and optional attributes values declared in each service type. This required some extra work on group objects.

Indeed, we used to have a minimum and a maximum number of objects. These were actually not really used, and developers misused this feature most of the time. They configured sight::data::ptr_vector with the same signature as sight::data::ptr which led to implicitly converting a true boolean meaning "optional data", in an integer meaning "at least one object". Thus the intent of the developer was actually the opposite of the actual result. The maximum number of objects was never used in real cases and was in fact introduced to solve an internal issue with non-XML apps. This was not a real need for the developer.

In the end, sight::data::ptr_vector now shares the same signature than sight::data::ptr. It is still possible to configure in the XML the auto-connect and optional on each member of the group, but this time the default value will be the one declared in the sight::data::ptr_vector declaration.

Last, fixing all of this highlighted some misconfiguration in a few services. These were corrected accordingly.

Closes #837 (closed)

Remarks

This MR was a nice case to use std::optional. Indeed, in many functions, we had an integer parameter allowing us to specify the index of the object in a group, if it is a group. We wanted to avoid duplicating all these numerous functions just to make the distinction of the group. Thus we had an ambiguous case for the 0 value, which could mean, "first element of the group", or "not a group". It was needed at some point in this coding phase to discriminate this, and switching to std::optional<int> was the best option, where now, the default value std::nullopt specifies "not a group".

How to test it?

  • Unit-tests have been modified accordingly. Of course, they should all pass.
  • You can run some applications to complete testing.
Edited by Flavien BRIDAULT

Merge request reports