-
Notifications
You must be signed in to change notification settings - Fork 325
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
fix load issue with scale tool #3110
Conversation
…iven as an absolute path, and the tool was created from a setup file. update error message to better reflect possible issues.
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.
An important workflow that is not exercised in tests is the GUI workflow. The GUI jumps through massive hoops to handle paths since working directory is the installation folder while files are sprinkled over disk(s). Also not sure how this handles files on different drives C:/D:/F: Will have to build and test or if already tested please indicate. Thank you
Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @aymanhab)
Thanks for the feedback @aymanhab. I haven't exercised any of those cases with the GUI, so that would be great if you could test since it seems like you've got a good idea for where that could break. The underlying SimTK functions have a bunch of logic to deal with different OS's, so there's a good chance it should be OK with different drive letters and such, but there's definitely some 🤞 there since I didn't test directly with different drives or in the GUI. |
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.
Tested the GUI on windows and seems to work.
Reviewed 2 of 3 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @carmichaelong)
* fix load issue scale tool load issue if model or marker set file is given as an absolute path, and the tool was created from a setup file. update error message to better reflect possible issues. * update CHANGELOG
#3125) * minor refactor to reduce num calls to get size of Array property * Avoid duplicate transformation to Ground frame when computing path length * remove_unused_variable * Remove macros that are replaceable by Vec3 operators * Inline usage of Macro in WrapEllipsoid * More refactoring, use det for determinant, vec operators instead of loops, inline method CalcDistanceSquaredPointToLine * Update continuous_integration.yml Do not use cached dependencies built before osx upgrade on github * Remove rather than comment out function that was moved to header and inlined. * Update GeometryPath.cpp Reuse variable for wrapSetSize * Update continuous_integration.yml Undo cache change since we verified build succeeds without it. * Use normSqr for consistency * Use UnitVec3 instead of Vec3 for plane normal * Avoid tweaking tangent points of WrapCylinder when single object wrapping, allow for optimization if disableVisualization is true. Fix test cases accordingly * Add MocoAngularVelocityTrackingGoal to bindings; add option for quaternion table in MocoOrientationTrackingGoal * Complete fixes for setRotationReference(); update changelog * fix load issue with scale tool (#3110) * fix load issue scale tool load issue if model or marker set file is given as an absolute path, and the tool was created from a setup file. update error message to better reflect possible issues. * update CHANGELOG * Add extendPostScale() to DeGrooteFregly2016Muscle (#3108) * Add extendPostScale() to DeGrooteFregly2016Muscle * Update changelog * Update continuous_integration.yml (#3115) * Update continuous_integration.yml Allow fresh build on osx to use latest github Action environment * Update continuous_integration.yml set CMake variable for OSX_DEPLOYMENT to 11 * Update continuous_integration.yml Restore mac osx-10.15 and target 10.10 for deployment * Fix inline assemble in catch.hpp dependency for ARM Macs (#3118) * * Fix inline assemble in catch.hpp dependency for ARM Macs See original change in catchorg/Catch2#1127 * Avoid tweaking tangent points of WrapCylinder when single object wrapping, allow for optimization if disableVisualization is true. Fix test cases accordingly * disableVisualization when creating MocoProblem for Model * Avoid messages about missing files if visualization is off. * fix comments per PR review * Disable visualization for all internal models used by MccoProblem * Fix typo/mistake in variable name * Couple other places to disable visualization for models internal to Moco and MocoInverse * improve comments per feedback on PR * Use finer grain includes per PR review * Retire Mtx::CrossProduct wrapper method for Vec3 operator * Remove now unused methods for CrossProduct, Interpolation, Translation, Rotation of data * Fix case for pathname of UnitVec.h * Use norm for Mtx::Magnitude * Remove redundant call to disableVisualization * Remove Mtx::Angle and inline references to it. * Replace Mtx::Normalize with Vec3.normalize * Update standard input for testMocoInverse This reverts commit fa4daa8. * Minor rearrangement per feedback on PR * Eliminate Mtx class, move remaining 2 functions to WrapMath * Inline replace DotProduct with operator, and remove from WrapMath.h, remove unused test case * Rename Normalize to NormalizeOrZero * Update WrapMath.h Test removal of zeroing output vector to bring NormalizeOrZero closer to normalize * Remove unused methods in WrapMath class, for consistency restore all WrapMath::NormalizeOrZero instances * Remove unused methods in WrapMath class, for consistency restore all WrapMath::NormalizeOrZero instances * Update std again based on latest wrapping code update * Update ModelProcessor.h Undo disableVisualization per feedback on PR Co-authored-by: Nicholas Bianco <[email protected]> Co-authored-by: carmichaelong <[email protected]> Co-authored-by: Keenon Werling <[email protected]>
Fixes issue #3109
Brief summary of changes
Updated how
GenericModelMaker::processModel()
figures out the path to the model file and marker set file. Also updated an error message in ScaleTool to be less misleading in certain cases.Testing I've completed
Ran usual tests locally. Also tested with different setup files that had absolute paths for model and marker set file.
Looking for feedback on...
Didn't add any test for this, but also couldn't think of a way to reliably add a test with an absolute path in a setup file.
CHANGELOG.md (choose one)
This change is