opensim-gui icon indicating copy to clipboard operation
opensim-gui copied to clipboard

Change ID tool panel to take an output path

Open sebastianskejoe opened this issue 5 years ago • 9 comments

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..

sebastianskejoe avatar Nov 22 '19 20:11 sebastianskejoe

@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 avatar Nov 22 '19 21:11 aymanhab

@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

aymanhab avatar Nov 25 '19 18:11 aymanhab

@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

sebastianskejoe avatar Nov 26 '19 08:11 sebastianskejoe

I wasn't sure if you need that to create the branch or to push the PR, anyway I invited you to our "organization" so you can follow the same workflow and don't necessarily need a fork of your own. Once accepted you can merge the PR into the branch and we take it from there.

Wonderful that you can see our zenhub board. Thanks for the info :+1:

On Tue, Nov 26, 2019 at 12:12 AM Sebastian Skejø [email protected] wrote:

@sebastianskejoe https://github.com/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 https://github.com/jimmyDunne as he interacts with users more on a daily basis. Please let us know what you think @jimmyDunne https://github.com/jimmyDunne

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/opensim-org/opensim-gui/pull/1157?email_source=notifications&email_token=AA6JY4GGWATQUIQTUYKX6JDQVTK7DA5CNFSM4JQVW2Z2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFFDSNI#issuecomment-558512437, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6JY4HS2NDBXBHHD4UIOVDQVTK7DANCNFSM4JQVW2ZQ .

aymanhab avatar Nov 26 '19 16:11 aymanhab

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

sebastianskejoe avatar Dec 04 '19 11:12 sebastianskejoe

@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.

aymanhab avatar Dec 05 '19 20:12 aymanhab

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

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

sebastianskejoe avatar Dec 13 '19 09:12 sebastianskejoe

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.

aymanhab avatar Dec 16 '19 19:12 aymanhab

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.

sebastianskejoe avatar Dec 17 '19 08:12 sebastianskejoe