opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
803 stars 324 forks source link

Component iterator of its subcomponents should be valid during Model building #536

Closed aseth1 closed 8 years ago

aseth1 commented 9 years ago

As it is now the iterator tree is only built at Model::connect(). We should discuss employing a suitable underlying std:: data structure instead of an adhoc linked list. In that way we could support inserts and deletes of subcomponents (as references) . A related problem is that most subcomponents are also properties, and a reference to a component in the property list is not guaranteed to be valid if the property list is edited (e.g. another element is added) even if the component itself is untouched. Guaranteering validity of a reference would allow us to have a valid iterator tree on every add such that the tree was iteratable immediately instead of waiting until connect (which guarantees properties will no longer be edited).

Relates to #418.

chrisdembia commented 9 years ago

How would the std:: data structure be notified of the updated property list?

aseth1 commented 9 years ago

We could require the use <ConcreteComponent>::add<C>() methods to append to the property list and also update the list of subcomponents (which would be the std::list or std::tree) e.g. Component::markAsSubcomponent. We would have to make the list property protected so users cannot upd_<prop_list>() directly otherwise they could be out of synch. Model already has add<C>() for all its subcomponent types (e.g. addBody(), addJoint(), ...) and these could also mark a component as a subcomponent instead of waiting until finalizeFromProperties. It does require the reference in the property list (or Set) to remain valid, however.

aseth1 commented 9 years ago

A more comprehensive solution is to enable Component to have a special property list (like we are doing for connectors) for all subcomponents intended to serialize them and to iterate over them. Then component builders would not have to define them as properties and then manually mark them as subcomponents - anything in that list is a subcomponent and serialized. Uber component's like Model then filter the large flat list using the component iterator based on type to handle Bodies, Joints, etc.. in required order. That would: 1) centralize/unify all all handling of subcomponents. A markAsSubcomponent() method could still exist to allow "hidden" subcomponents (however, there are no examples of that atm). 2) it would eliminate most of the use of sets except for the notion of groups. If subcomponent list could be easily grouped (which is trivial for types) then we could also have model files more or less preserved by grouping all Bodies, Joints, Constraints, etc... together, and also allow convenient access to useful sublists. Again, this may be better handled if Component also has a notion of membership to groups, then an iterator filter could be used to find Components that have the same group affiliation.

aymanhab commented 9 years ago

@aseth1 I think we should step back and come-up with a design for how Models/Submodels/components will get-assembled. Then we can decide if a model is a special component that can maintain a tree of components (we can write/document what are the assumptions about the model/tree/ownership, when are components wired/added) or if a model is a generic component like others which means the tree stays distributed among components/submodels and we have to decide using some other way when to say we're done assembling the "uber" model, using finalize and other mechanisms.

aseth1 commented 9 years ago

To me traversal of components must come before "wiring" of components. We need the ability to iterate through components before anything is finalized and any connections can be made. Currently, we can only traverse components after the uber connect() builds the tree - that is way too late. If each Component can traverse its subcompents as soon as the component is added, that will be useful for all other activities to follow (model building by getting Components of particular type, resolving connections, adding components to the computational system, etc ...). There is no need for reconsidering the component design - we need to a solution to easily access/traverse components so the design can work better!

aseth1 commented 9 years ago

we can decide if a model is a special component that can maintain a tree of components (we can write/document what are the assumptions about the model/tree/ownership, when are components wired/added) or if a model is a generic component like others which means the tree stays distributed among components/submodels

We should try our best to do away with any component having to be special and be the owner of a tree. Any component should be able to traverse its subcomponents immediately, once added or designated as such, regardless if it is a Model, or a Force. If it has subcomponents you should be able to iterate over them and filter by type, etc...

chrisdembia commented 9 years ago

We should try our best to do away with any component having to be special and be the owner of a tree.

I think this is a kind of thing that can't be a partial "we should move away". It has to be "we are completely changing to no set tree owner", which then requires convincing use cases. An environment class is a pontentially convincing use case, if we want Models to be able to stand on their own or to be part of an Environment.

Something that I am unable to keep track of when we discuss prospective changes is whether they are for 4.0 or not. Is this "no-tree-owner" a 4.0 change? Seems like we are getting to a point where we really need to prioritize.

Then component builders would not have to define them as properties and then manually mark them as subcomponents - anything in that list is a subcomponent and serialized.

This would resolve a confusion I have about why we are keeping track of such objects twice. However, it seems like there is a good reason for the separation between properties and subcomponents. Properties are changeable, while I may have a component that has fixed subcomponents (e.g., a controller that has a delay subcomponent; I don't want users to be able to remove it as a subcomponent; so I'd have it as a member; see my delay PR for this example).

SO, if we allow a special subcomponent-property, we must also allow subcomponents that are not generated via the special subcomponent-property.

aseth1 commented 9 years ago

@chrisdembia

I think this is a kind of thing that can't be a partial "we should move away". It has to be "we are completely changing to no set tree owner",

I am not certain tree ownership is the essential problem. The problem is when is Component.getComponetList() useable? Model or uber component could own the tree for whatever reason (e.g performance), but as long as subcomponents of any Component can be traversed (even slowly) before I connect the model, i would be satisfied.

which then requires convincing use cases.

This seems to be basic missing functionality. You instantiate a Component and you want to know/list all its subcomponents. You want to set a value on a subcomponent or on components of a given type. I ran into this trying to automate the satisfaction of dependencies. I wanted to walk through the Connectors of components and instantiate the dependency types and add them to the Model. I could not do this because I could not traverse the subcomponents without connecting the Model, which of course failed because my dependencies were unsatisfied. Had the tree been traversable after finalizeFromProperties, it would have been workable.

I can imagine traversing subcomponents and their connectors as being critical for GUI editing/model building. You pull the component off a palette in the GUI, How will the GUI tell you what that Component is dependent on to work? Expose the Connectors? What about the connectors of is subcomponents? You will not be able to connect a Model in order to ask what are my subcomponents without already knowing what the dependencies are and having them satisfied.

Something that I am unable to keep track of when we discuss prospective changes is whether they are for 4.0 or not. Is this "no-tree-owner" a 4.0 change? Seems like we are getting to a point where we really need to prioritize.

It is of high priority for 4.0 IMHO, but we decide priority as a team.

SO, if we allow a special subcomponent-property, we must also allow subcomponents that are not generated via the special subcomponent-property.

I agree. I wrote:

A markAsSubcomponent() method could still exist to allow "hidden" subcomponents (however, there are no examples of that atm)

I am glad your Delay component is now an example!

chrisdembia commented 9 years ago

Oh sorry I missed your markAsSubcomponent; that would be great. I'm not sure how it would work, but I like the interface of calling that method. It would be called during connect?

sherm1 commented 9 years ago

Ajay, Shrinidhi, and I discussed this and came up with a plan that we think everyone will like: basically get rid of subcomponents by creating the concept of "hidden properties". That means the model tree would go back to being composed of just components and properties, and then every property that contains a component would imply that it behaves like a subcomponent, that is, its methods get executed properly.

aymanhab commented 9 years ago

Thanks @sherm1 for keeping everybody in the loop. It would be great to document what was the problem you're solving, the solution, and the resulting changes to the API for the benefit of people who were not in the discussion but will be using the model building API.

sherm1 commented 9 years ago

It would be great to document what was the problem you're solving, the solution, and the resulting changes to the API

Just an idea so far. Let's discuss at Thursday's meeting.

jenhicks commented 8 years ago

@aseth1 Can we close this issue given that PR #740 was merged?