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

Make Frame frame_geometry a member subcomponent. #2338

Open
wants to merge 60 commits into
base: main
Choose a base branch
from

Conversation

chrisdembia
Copy link
Member

@chrisdembia chrisdembia commented Oct 29, 2018

This PR is in response to @aseth1 's feedback in #2310 that the FrameGeometry that comes with Frames should not be exposed.

This should not be merged until after #2310 is merged.


This change is Reviewable

chrisdembia and others added 30 commits October 4, 2018 10:08
This file was previously updated to XML version 30516 but we do not
support this intermediate file version. I reverted the file back to its
previous 30000 version and repeated the following changes:

- changed minimum_activation for all muscles to 0.0.
- changed min_control for all muscles to 0.0001.

These changes are required for testStaticOptimization to pass.
For min_control, I wanted to use 0.0 but updateFromXMLNode() strips away
min_control if its value is 0.0 because it assumes it was an error
from old model files.
chrisdembia and others added 26 commits October 20, 2018 02:12
Co-Authored-By: chrisdembia <[email protected]>
Co-Authored-By: chrisdembia <[email protected]>
…path

Remove model name from absolute path, and use absolute path (sometimes) for connectee path
@aymanhab
Copy link
Member

@chrisdembia The reason this and some other geometry components were exposed in the first place is to allow modification of display options e.g. show/hide or scale and to make these changes persistent. As of now some of these options may not work for FrameGeometry but to remove it completely from XML and then try to find a place to include a separate Appearance object (similar to how we do for Wrap Objects) seems a bit of an incomplete solution. Sorry I didn't explain in my earlier feedback.

If we can get the XML to be hidden because all the contents are the same for all FrameGeometry then that would reduce the clutter without affecting functionality though I don't know if that's possible or important at this point.

@chrisdembia
Copy link
Member Author

@aymanhab I understand, and I agree we want to be able to expose certain properties of the FrameGeometry. I was thinking what you were saying: add in properties for the specific things we want to be able to customize. I figure it will be easier to add in such customization later rather than expose frame_geometry now and require writing updateFromXMLNode() code later.

I am fine either way. It's up to the group if we want to merge this.

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.

4 participants