search

opensim-org / opensim-core

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

Inside Component::getComponent(), invoke setParent() on returned component. #1393

Open chrisdembia opened 7 years ago

chrisdembia commented 7 years ago

Right now, if one makes a coy of a Component, they should call finalizeFromProperties() before accessing any subcomponents: the subcomponents won't have their parent pointer set until finalizeFromProperties() is called.

A similar issue arises for connectors and inputs, which we solve in #1340 by simply invoking setParent() inside of Component::getConnector().

For consistency, we should also do the same thing for Component::getComponent(), Component::getComponentList(), etc.

Here is some of the discussion from #1340:

@chrisdembia said: These are the options I've thought of so far:

  1. Prohibit the concise syntax above and require calling finalizeFromProperties().
  2. Cause the same behavior for subcomponents; e.g., have getComponent(), getComponentList(), etc invoke setParent() before returning the subcomponent.
  3. When a user calls Geometry::setFrame(), or more generally sets up connections via the API, do not store the connectee name at that time; hold onto the given reference. Only fill in the connectee_name property when serializing, etc. This change would have other benefits too (avoid issues with connecting to an object that isn't part of the model yet). However, this is also out of scope of this PR.

@aseth1 said: I see now, thanks. My preference then is 2. if it works consistently. I also think 3. would be great and addresses the ability to rename components. If the reference is the connection then we could extract the name for the purpose of serialization. For now let's leave your solution for Connectors/Inputs. After the PR is merged lets try 2. to establish consistency with components. Let's put 3. on the back-burner, for now.

chrisdembia commented 7 years ago

In #1340, @aymanhab said:

Having a side-effect inside a getter method (that users would assume is const/safe) is less desirable, so I'd rather see option 3 but that's futuristic for now.

chrisdembia commented 7 years ago

Note: when solving this issue perhaps by not storing connectee names until serialization, remove the doxygen comments for Component::getInput(), etc. that describe the side effect of updating the input's or connector's owner.