-
Notifications
You must be signed in to change notification settings - Fork 35
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
base: expose_idtool_forces_file
Are you sure you want to change the base?
Change ID tool panel to take an output path #1157
Conversation
@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 |
@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 |
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.
Yes, I have read access to the zenhub development board.
|
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 👍
…On Tue, Nov 26, 2019 at 12:12 AM Sebastian Skejø ***@***.***> 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
<#1157?email_source=notifications&email_token=AA6JY4GGWATQUIQTUYKX6JDQVTK7DA5CNFSM4JQVW2Z2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFFDSNI#issuecomment-558512437>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA6JY4HS2NDBXBHHD4UIOVDQVTK7DANCNFSM4JQVW2ZQ>
.
|
I've accepted the invitation to the opensim organization, but it seems that I still don't have access to push the branch:
|
@@ -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"/> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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”
@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. |
I have changed the text to "Generalized forces file". I am still not able to create branches in this repo. |
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. |
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. |
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)