robofit / arcor2

Solution for end-user programming of (collaborative) robots using Augmented Reality. From AR to Python and back!
GNU Lesser General Public License v3.0
15 stars 16 forks source link

Object parameters and project parameters of type string without `\"` prefix and postfix #787

Open magiino opened 2 years ago

magiino commented 2 years ago

We would like to simplify string parameters in system. The need of prefixing and postfixing string parameter with \" - "value": "\"AuboI5Gripper.robotconfig\"" was created with use of json.loads("") which is used for converting string to correct data type. We do not even need type provided in parameter datamodel. But the drawback of this is that strings must be prefixed and postfixed with \".

Both object parameter and project parameter has knowledge about type in their data model. ObjectParameter

{
  name: string,
  type: string,
  value: string
}

ProjectParameter

  id: string,
  name: string,   // Gets or sets name of parameter for usage in the script.
  type: string,
  value: string,
  displayName: string,  // Gets or sets localized name of the parameter that can be displayed in UI.
  description: string,
  range?: Range

Solution would be to use json.loads("") just for non string parameters. With this we can ommit prefixing and postfixing string parameters with \".

Example code how we deal with this issue on our side with project parameters.

 def get_parameter(self, id: str) -> Any:
        parameter = self.resources.project.parameter(id)
        if parameter.type == "string":
            return parameter.value

        return json.loads(parameter.value)

But object parameters are parsed on Arcor2 side when scene is loaded in Resources class. See link to issue - link.

What is your opinion? I would make PR if we agree.

One more question comes to my mind. What changes needs to be done in AR editor? AR editor can put objects to scene and set their parameters. Also is working with project parameters, I gues PR is needed also here.

ZdenekM commented 2 years ago

Well, I'm not sure if handling the string parameter value differently than other types would simplify things or complicate them. Sure, for string parameters, the current state might seem more complicated than it could be. But the approach is the same for all types - there is consistency. Handling one type of parameter in a different way than others sounds to me like a source of potential trouble.