Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design discussion for Parameterization of VisualStates #2

Open
sudo-panda opened this issue Jun 10, 2019 · 21 comments
Open

Design discussion for Parameterization of VisualStates #2

sudo-panda opened this issue Jun 10, 2019 · 21 comments

Comments

@sudo-panda
Copy link
Collaborator

I have already added parameterization to VisualStates tool but as Sir Okan said, and I agree, that it would better to discuss it with everyone to better the design.

Here are some screenshots of the current design:
image
image
image
image

Pull request #1 adds this feature to VisualStates

@pushkalkatara
Copy link
Member

I checked the design for parameterization, It looks good, let's develop parameters taking the easiest example of Kobuki Obstacle Avoidance, further we'll iterate on the design with the help of examples. I believe we have the parameter for LaserThreshold in the example, thus we can develop using it.

@sudo-panda
Copy link
Collaborator Author

kobuki_obstacle_avoidance.txt
This the xml file for the modified kobuki obstacle avoidance. I changed the extension to txt so the I can upload it here.

@pushkalkatara
Copy link
Member

Hi @sudo-panda. I tried importing the behavior. I have the following reviews -

  1. The list showed after import only describes the parameter details. We have a current PR open on VisualStates repo to select particular states to import. The idea is to give details on a single tab after import of all the properties of the state. Thus we can discuss it more.
  2. The parameters tab in the data option looks good. Your suggestion with tabbed UI would make the GUI more intuitive and easy for the user to alter the parameters, thus you can work further on that.

@okanasik
Copy link
Collaborator

First of all, I mean the design of the functionality. We need to discuss a little how the feature will work and how we can implement this feature as part of the current visualstates.

  • I checked your pull request, you designed params as part of visualstates. However, it is more intuitive to think that parameters are part of states. This would also make more sense when we import states with parameters.
  • As a small feedback for UI, it would be better, if you add new button at the top of the parameter. Also, it would look more professional if we can create a single look-and-feel of the UI. Can you check the ROS config UI? You can try to match the look similar to those UIs so that users have a same feeling for whole tool.
  • I think, it is not a good practice to catch index error by try except block, when we can explicitly check whether xml the node and attributes exists.
  • How do you represent parameters in source codes, maybe we can design a one-to-one mapping of a state variable as parameter. Therefore, when programming, we do not inject some random syntax to refer to the parameters. What do you think?

@sudo-panda
Copy link
Collaborator Author

States already have variables individually so why create a new way to add state wise variables. We can just convert the variables to act the way parameters do now. I had initially thought of parameters as a part of VisualStates and not individual states because I thought that parameters that needed to be tweaked were usually used globally in various states. We could also make the parameters global and mention during import the parameters used by each state. What do we do?

I agree that we should make the UI similar everywhere in the whole tool and I will place the new button at the top and try to make other changes that feel suitable.

@okanasik
Copy link
Collaborator

Since, we aim to import particular state from a visualstate, individual parameterization of whole visualstate does not make sense. Parameters can be thought in that sense a public variable of the state/behavior. For example, if parameters are at the root state, they will be used in whole visualstate. However, I am not sure which approach is better. Let's discuss this further by considering the case where the use wants to import a particular state of a complex behavior.

@pushkalkatara
Copy link
Member

pushkalkatara commented Jun 17, 2019

Currently, in VisualStates we have a Global Namespace which contains the ROS parameters and global variables which are accessible by any state or transition. Each state also contains a Local Namespace in which variables only accessible by that particular state exists. A variable, whether defined in the global namespace or local namespace may act as a parameter. But I think it is not necessary that every variable declared by the user is a parameter. I also think it is a good topic for discussion on whether to keep parameters individually or state specific. I have two examples in mind -

  1. In the current implementation of Kobuki Obstacle Avoidance behavior, the obstacle_threshold is in Local Namespace i.e only accessible by the states under the root states.
  2. PID controller for Prius Examples: Prius car has PID controller in throttle and steering. Both the PID controllers have separate Kp, Ki, kd thus they are separate parameters and apply individually to that state.
    My vote also goes to parameters being part of states, as it would benefit in complex behaviors and their visualization. Also, it would reduce name conflicts between parameters.
    We can think of a case in which making a global variable a parameter might be helpful, but I think it would increase the complexity to develop a behavior and understand the tool.

@sudo-panda
Copy link
Collaborator Author

Ok so we make parameters a local namespace variable in the source code. Do we only show the list of parameters during import or allow them to be modified?

@sudo-panda
Copy link
Collaborator Author

Final design:

  • Parameters are now part of local namespaces and is to be added to the local namespace dialog.
  • They act like normal variables in the local namespace.
  • Importing them do not require them to be displayed any longer.

@sudo-panda
Copy link
Collaborator Author

Right now the parameters behave exactly like local variables except for how they are created. So they don't have much advantage over the local namespace variables. This will change when we added the selective import of states and the online importer exporter to the tool.

Since I have to show the use of parameters and their advantage in the demonstration video I have nothing to show. Hence I was wondering if I could add a dialog that comes up during the generation of code that lists out all the parameters along with their states and allows users to only change the values. This will make it easier for the users to tweak their values to perfection

@sudo-panda
Copy link
Collaborator Author

Video for parameterization of VisualStates

@sudo-panda
Copy link
Collaborator Author

sudo-panda commented Jun 30, 2019

@okanasik You asked me to display a list of parameters added on import of a VisualStates file. Do I just show them or let the users edit the value too?

@okanasik
Copy link
Collaborator

okanasik commented Jul 4, 2019

@sudo-panda I am not sure whether letting the user to change the parameters while importing the state will be useful. The user may need to test the behavior to find the correct parameters. What do you think?

@jmplaza
Copy link
Contributor

jmplaza commented Jul 5, 2019

The auotmata parameters is more an edition topic than an import topic. Isn't it?

I mean, the user imports an automaton (an existing HFSM from the library) to include it on her new automaton that is being created, and AFTER that import she can fine tune the particular behavior of the imported automaton changing its parameters at will. This remains me like importing a software library: the programmer imports a library and after that calls its functions with the corresponding parameters/arguments....

@sudo-panda
Copy link
Collaborator Author

So what are you suggesting @jmplaza ?

@sudo-panda
Copy link
Collaborator Author

Imported Params

Is this representation fine?

@okanasik
Copy link
Collaborator

okanasik commented Jul 9, 2019

@sudo-panda this representation looks good. Nice work.

  • It could be better if you can add some background color to differentiate the parameters from the window background.
  • Also it could be better to show parameters as table so that you do not repeat Type, Value and Description for every line. Type, Value, Description can be bold at the line of bold Parameter part.
    Best

@okanasik
Copy link
Collaborator

okanasik commented Jul 9, 2019

The auotmata parameters is more an edition topic than an import topic. Isn't it?

I mean, the user imports an automaton (an existing HFSM from the library) to include it on her new automaton that is being created, and AFTER that import she can fine tune the particular behavior of the imported automaton changing its parameters at will. This remains me like importing a software library: the programmer imports a library and after that calls its functions with the corresponding parameters/arguments....

Actually, we do not design this way. We assume that parameter update and change is part of the import process. Also, user can change the parameter at any point of the development. The only difference is that user can import states from a repository of behavior. Also, the user can cherry-pick which ever state, she wants to import.

@pushkalkatara
Copy link
Member

Imported Params

Is this representation fine?

  • The representation looks good. I think maybe we can discuss the visuals in the case of large automatons.
  • Also, when would this representation be shown? Is it right after imports, or would we provide the user an option to show the description?

@sudo-panda
Copy link
Collaborator Author

Week 7 discussion:
Tree of states with parameters are going to be shown during import and the the states can be selected right there and states are collapsible

@pushkalkatara
Copy link
Member

Hi @sudo-panda , I found a minor bug while checking the PR. Here's the issue - #6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants