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

Add parameters to visual states tool. #1

Merged
merged 33 commits into from
Aug 29, 2019
Merged

Add parameters to visual states tool. #1

merged 33 commits into from
Aug 29, 2019

Conversation

sudo-panda
Copy link
Collaborator

This feature includes the following:

  • A dialog to Add/Edit/Remove parameters.
  • Saving of parameters during saving of VisualStates File.
  • Loading of parameters when opening VisualStates File.
  • Importing of parameters when importing VisualStates File.
  • Resolving conflicts where parameters have the same name while import.
  • Replacing parameters with their values when generating Python/C++ Code.

@pushkalkatara
Copy link
Member

Hi @sudo-panda . I was trying testing the above changes. I am facing the error below -

Traceback (most recent call last):
  File "/home/pushkalkatara/catkin/src/colab-gsoc2019-Baidyanath_Kundu/scripts/main.py", line 24, in <module>
    from visualstates.gui.visualstates import VisualStates
  File "/home/pushkalkatara/catkin/src/colab-gsoc2019-Baidyanath_Kundu/src/visualstates/gui/visualstates.py", line 30, in <module>
    from .dialogs.paramsdialog import ImportedParamsDialog
  File "/home/pushkalkatara/catkin/src/colab-gsoc2019-Baidyanath_Kundu/src/visualstates/gui/dialogs/paramsdialog.py", line 25, in <module>
    from src.visualstates.gui.tools.elidedlabel import ElidedLabel
ImportError: No module named src.visualstates.gui.tools.elidedlabel

What might be the issue?

@sudo-panda
Copy link
Collaborator Author

Seems like ROS doesn't allow absolute imports. It should be fixed now.

@pushkalkatara
Copy link
Member

Yes, it works now. please follow this convention in other files too.

@pushkalkatara
Copy link
Member

Please share the most recent XML file of Kobuki Obstacle Avoidance behavior which is supporting parameters.

@sudo-panda
Copy link
Collaborator Author

The file is available as a gist here

@pushkalkatara
Copy link
Member

The import interface with selective state and parameter description together looks nice.
One thing I would like to point out is the description of the parameter is not completely visible nor there is an option to read it. This needs a fix.
parameter

@sudo-panda
Copy link
Collaborator Author

The import interface with selective state and parameter description together looks nice.
One thing I would like to point out is the description of the parameter is not completely visible nor there is an option to read it. This needs a fix.
parameter

If you hover over the description you can see the whole description for now

@sudo-panda
Copy link
Collaborator Author

The import interface with selective state and parameter description together looks nice.
One thing I would like to point out is the description of the parameter is not completely visible nor there is an option to read it. This needs a fix.
parameter

I have added a way to show the parameters.

@okanasik
Copy link
Collaborator

Dear @sudo-panda, there is a small error in handling the id based parameters. To reproduce the error:

  • add a parameter
  • add another parameter
  • remove the first parameter
  • remove the other parameter
    At that stage we get the following error:
Traceback (most recent call last):
  File "/home/okan/jderobot/sudopanda-repo/catkin_ws/src/visualstates/src/visualstates/gui/dialogs/namespacedialog.py", line 289, in removeHandler
    self.emitParams()
  File "/home/okan/jderobot/sudopanda-repo/catkin_ws/src/visualstates/src/visualstates/gui/dialogs/namespacedialog.py", line 319, in emitParams
    params.pop(id)
IndexError: pop from empty list

You assume that the initial ids are same always and add new variables based on the size of the list. However, if you remove a parameter in the list and add a new parameter, you have two parameters having the same id. I think it could be better strategy to keep paremeter objects indexed/keyed by the parameter name which is supposed to be unique. That way you will have a unique key to access all parameters and you will get rid of struggling of integer ids that you make up.

@okanasik
Copy link
Collaborator

@sudo-panda your fix handles the crash, but after removing all the parameters and closing the dialog and reopening the dialog shows me the parameters that are supposed to be deleted. As I said in my previous comment, can you reimplement handling of the parameters. You do not need artificial integer indexes as id. They complicate the code. Since the parameter name is supposed to be unique, you can use them to store parameters in dict keyed with name.

@sudo-panda
Copy link
Collaborator Author

The dictionary of parameters seems odd as parameter is an object which already has a name property so I changed only the Namespace Dialog to use a dictionary to store parameters. It makes sense there due to the need of accessing each parameter separately in random order. Whereas everywhere else it makes no difference as to them being accessed serially or in random order as all of them are accessed one after the other.

@pushkalkatara
Copy link
Member

pushkalkatara commented Aug 9, 2019

Found a minor bug in the PR. #6

Copy link
Member

@pushkalkatara pushkalkatara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, please remove the .idea files or add them to .gitignore.

@pushkalkatara
Copy link
Member

  • [Feature] we can display the accepted naming conventions for the parameters while displaying the parameter name error.

@pushkalkatara
Copy link
Member

@sudo-panda The .idea files are still redundant. These files are software dependent files and we don't need it in the tool. Could you please remove them.

@sudo-panda
Copy link
Collaborator Author

I am working on it right now.

@pushkalkatara
Copy link
Member

Also @sudo-panda , we need to display the naming conventions for the parameters while displaying the parameter name error.

Rest the PR seems great. Good work!

Could you sum up the parametrization and the import-export library PRs and make a comprehensive PR on the main VisualStates repository. https://github.com/jderobot/VisualStates

@sudo-panda sudo-panda merged commit 1d44a54 into master Aug 29, 2019
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

Successfully merging this pull request may close these issues.

3 participants