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

Change ID tool panel to take an output path #1157

Open
wants to merge 2 commits into
base: expose_idtool_forces_file
Choose a base branch
from

Conversation

sebastianskejoe
Copy link

Fixes issue #1146 (the part about the inverse dynamics tool)

Brief summary of changes

Instead of choosing a directory output, users must choose output file for the generalized forces. Currently the ID tool always writes an inverse_dynamics.sto file to the chosen directory, which will overwrite the result of previous ID analyses if users are not careful to rename the file before running the ID tool again.

I assume the current behavior is due to the ID tool being able to write two files (generalized forces and body forces), but I don't believe the ability to write body forces is exposed in GUI, although I might be wrong. If that is true, this change does not introduce limited capabilities of the GUI, but IMO makes it more intuitive to use.

Another (personal) objective was to get started with opensim-gui development, which already was fulfilled, so feel free to reject this PR, if the proposed behavior is not wanted.

Testing I've completed

Tested on Windows 10.

CHANGELOG.md (choose one)

  • NOT updated as I'm not entire sure what the convention about updating the changelog is..

@aymanhab
Copy link
Member

@sebastianskejoe Wonderful to have you contributing to this opensim-gui, welcome aboard 👍

Will review the specific code changes and let you know. Feel free to open other PRs as you feel more comfortable with the development environment. Welcome again

@aymanhab
Copy link
Member

@sebastianskejoe Can you create a branch of opensim-gui repo rather than your fork so I can easily switch to it and test locally? Also can you install the zenhub plugin for Chrome https://www.zenhub.com/extension to see if you can access our development boards? Thank you.

I agree that selecting the file is definitely more common but I'll also pull in @jimmyDunne as he interacts with users more on a daily basis. Please let us know what you think @jimmyDunne

@sebastianskejoe
Copy link
Author

@sebastianskejoe Can you create a branch of opensim-gui repo rather than your fork so I can easily switch to it and test locally?

Doesn't this require that I have access to push my branch to the opensim-gui repo? Otherwise I'm not sure how to do this.

Also can you install the zenhub plugin for Chrome https://www.zenhub.com/extension to see if you can access our development boards? Thank you.

Yes, I have read access to the zenhub development board.

I agree that selecting the file is definitely more common but I'll also pull in @jimmyDunne as he interacts with users more on a daily basis. Please let us know what you think @jimmyDunne

@aymanhab
Copy link
Member

aymanhab commented Nov 26, 2019 via email

@sebastianskejoe
Copy link
Author

I've accepted the invitation to the opensim organization, but it seems that I still don't have access to push the branch:

au323720@D18814 MINGW64 ~/Development/opensim-gui (ID_tool_file_output)
$ git push origin ID_tool_file_output
Fatal: HttpRequestException encountered.
Username for 'https://github.com': sebastianskejoe
remote: Permission to opensim-org/opensim-gui.git denied to sebastianskejoe.
fatal: unable to access 'https://github.com/opensim-org/opensim-gui.git/': The requested URL returned error: 403

au323720@D18814 MINGW64 ~/Development/opensim-gui (ID_tool_file_output)
$ git remote get-url origin
https://github.com/opensim-org/opensim-gui.git

@aymanhab aymanhab changed the base branch from master to expose_idtool_forces_file December 5, 2019 20:50
@@ -252,7 +252,7 @@
<SubComponents>
<Component class="javax.swing.JLabel" name="jLabel11">
<Properties>
<Property name="text" type="java.lang.String" value="Directory"/>
<Property name="text" type="java.lang.String" value="Storage file"/>
Copy link
Member

Choose a reason for hiding this comment

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

I would not call this "Storage file" as it is generic name, I'd go with "Forces file" or "Generalized forces file" if space allows. Otherwise looks good.
Any thoughts or feedback @jimmyDunne or @chrisdembia

Copy link
Member

Choose a reason for hiding this comment

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

I like “generalized forces file”

@aymanhab
Copy link
Member

aymanhab commented Dec 5, 2019

@sebastianskejoe I'm trying to set the permissions to allow you to create/push to a branch, in the mean time I changed upstream to a branch I created, once done we can merge this branch instead of your fork's. I changed your role to team member now instead of collaborator, please let me know if that works and you can create branches on your own.

@sebastianskejoe
Copy link
Author

I have changed the text to "Generalized forces file".

I am still not able to create branches in this repo.

@aymanhab
Copy link
Member

Thanks @sebastianskejoe much appreciated, can you please try again to create a branch? It's taking much longer than I thought I just want to make sure you have a solid pipeline to contribute so future ones go faster.

@sebastianskejoe
Copy link
Author

Works now - I have created a new branch, but not a new PR, since the pushed branch is the same as my fork. Let me know if I should create a new PR as well.

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