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

[SofaPython3] Add a LinkPath object in both the binding and plugin. #265

Merged
merged 7 commits into from
Aug 16, 2022

Conversation

damienmarchal
Copy link
Contributor

@damienmarchal damienmarchal commented Jun 9, 2022

This PR is following the discussion in #197

The existing syntax to make links in SofaPython3 is the following one:

node.addObject("MeshObjLoader", name="loader")
node.addObject("MechanicalObject", position=node.loader.position.getLinkPath())

In addition to be very verbose the getLinkPath() is returning a string (eg: "@/node/loader.position").
Representing linkpath with string has the drawback of forcing the two objects to be part of the same simulation graph which is not always the case.

Example of code that does not work as expected:

def MyPrefab(target):
	node = Sofa.Core.Node("MyPrefab")
	node.addObject("MeshObjLoader", name="loader")
	node2.addObject("MechanicalObject", position=target)
	return node

def createScene(root):
	root.addObject("MeshObjLoader", name="loader")
	root.addChild( MyPrefab(loader.position.getLinkPath()) )

It does not work because string returned by getLinkPath() is a path query into the scene-graph to locate the object or the data
field to attach not into the one of the graph the destination object is.

To solve this issue this PR introduce a LinkPath object which is dedicated to point to a sofa object.
This LinkPath object is exposed in python and can be used to make parenting between objects and data without the need of graph to path then path to graph conversion (so it is also much faster.. but we don't really care here right ?).

For consistency with the "value" in data field the PR expose the LinkPath with a dedicated "linkpath" attribute.
So the code is now the following:

root.addObject("MeshObjLoader", name="loader")
root.addObject("MechanicalObject", name="p1", position=root.loader.position.value)      # Copy the value
root.addObject("MechanicalObject", name="p2", position=root.loader.position.linkpath)   # Make a link between data without the need for string based query

root.addObject("Mapping", name="loader", input=p1.linkpath)

@damienmarchal damienmarchal added enhancement New feature or request pr: status wip labels Jun 9, 2022
@damienmarchal damienmarchal force-pushed the pr-add-linkpath-object branch 6 times, most recently from 95fec4d to 071000f Compare June 13, 2022 22:53
@damienmarchal
Copy link
Contributor Author

damienmarchal commented Jun 21, 2022

This PR really deserve some attention because the use of string based link is very constraining :)

Plugin/src/SofaPython3/LinkPath.cpp Outdated Show resolved Hide resolved
bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp Outdated Show resolved Hide resolved
bindings/Sofa/src/SofaPython3/Sofa/Core/Binding_Node.cpp Outdated Show resolved Hide resolved
The existing syntax to make links in Sofa is the following one:
```python
node.addObject("MeshObjLoader", name="loader")
node.addObject("MechanicalObject", position=node.loader.position.getLinkPath())
```

In addition to be very verbose the getLinkPath() is returning a string (eg: "@/node/loader.position")
Representing linkpath with string has drawback as it force the two objects to be part of the
same simulation graph which is not always the case.

Example of code that does not work as expected:
```python
def MyPrefab(target):
	node = Sofa.Core.Node("MyPrefab")
	node.addObject("MeshObjLoader", name="loader")
	node2.addObject("MechanicalObject", position=target)
	return node

def createScene(root):
	root.addObject("MeshObjLoader", name="loader")
	root.addChild( MyPrefab(loader.position.getLinkPath()) )
```
It does not work because getLinkPath is returning a string, this string is then
used to makes a path query into the scene-graph to locate the object or the data
field to attach to.
But in this case as the object issuing the query is not in the same graph
the query fails.

To solve this issue this PR introduce a LinkPath object which is pointing to a sofa object.
This LinkPath object is exposed in python and can be used to make parenting between objects and data
without the need of graph to path then path to graph conversion (so it is also much faster..
but we don't really care here right ?).

For consistency with the "value" in data field the PR expose this link path
with a dedicated "linkpath" attribute.
So the code is now the following:
```python
root.addObject("MeshObjLoader", name="loader")
root.addChild("MechanicalObject", name="p1", position=root.loader.position.value)      # Copy the value
root.addChild("MechanicalObject", name="p2", position=root.loader.position.linkpath)   # Make a link between data without string query

root.addObject("Mapping", name="loader", input=p1.linkpath)
```
@damienmarchal damienmarchal force-pushed the pr-add-linkpath-object branch 2 times, most recently from eb83067 to 2484296 Compare June 21, 2022 21:06
@hugtalbot
Copy link
Contributor

@damienmarchal do you aim at deprecating getLinkPath() ?
if so, we should make a compatibility layer informing users

Examples of SP3 could be also updated with this new API

@damienmarchal
Copy link
Contributor Author

damienmarchal commented Jun 22, 2022

Given that str(myobj.linkpath) produce the same as myobj.getLinkPath() yes we could safely deprecate getLinkPath() (and we probably should).

But given:

  • LinkPath is a completely new feature,
  • getLinkPath() is used everywhere,
  • we are already annoying a lot our users because of the bunch of NG changes ...

The transition workflow I think is wise would be:

  • step1: add the new feature (which already implement a compatibility layer),
  • step2: see if there is no bad feedback from ones that starts to use out of our code base and if conclusive...
  • step3: update the SP3 examples for 22.12
  • step4: adds the transition message for 22.12

damienmarchal and others added 2 commits June 22, 2022 14:19
Co-authored-by: Alex Bilger <alxbilger@users.noreply.github.com>
@damienmarchal
Copy link
Contributor Author

It seems that MacOS CI is broken

@damienmarchal
Copy link
Contributor Author

@hugtalbot so what ?

@damienmarchal
Copy link
Contributor Author

damienmarchal commented Jun 29, 2022

This PR is adding a new feature linkpath that is a "non string" approach to make link between data or objects.
The PR was proposed to implement the conclusion of #197
The conclusion of #197 was to be explicit so doing

node.addObject("toto", position=otherObject.position.linkpath)  # makes a link without path conversion explicitely
node.addObject("toto", position=otherObject.position.value)  # makes a copy to initialize

instead of:

node.addObject("toto", position=otherObject.position)  # makes a link without path conversion implicitely
node.addObject("toto", position=otherObject.position.value)  # makes a copy to initialize

Hugo suggested to deprecate getLinkPath()... I'm not sure it is the proper PR to do that because it is not wise to deprecate a long standing feature by a modernized replacement.

There are an additional point of dicussion:
JérémieA pointed that in InSimo's fork they are using "path" in place of "getLinkPath()". To have compatibility it may be wise to deprecated "getLinkPath()" in favor of "path".

# The existing version in SofaPython3... is to makes a link with a stringyfied path (so generation and parse) and to work this imply being in the same graph for path resolution suceed at init and so on). 
node.addObject("toto", position=position.getLinkPaht()) 

# The existing version in InSimo is functionally strictly equivalent to getLinkPath() except that it use a python attribute to make the syntax more "slick"
node.addObject("toto", position=position.path)  

# The version in this PR is *not* a replacement of getLinkPath it is superset.
node.addObject("toto", position=position.linkpath)  # makes a link without path conversion
node.addObject("toto", position=str(position.linkpath))  # makes a link with path conversion

An open question is wether we prefer to do:

node.addObject("toto", position=position.linkpath)  # makes a link without path conversion
node.addObject("toto", position=str(position.linkpath))  # makes a link with path conversion

or to add a "shortcut" for str(position.linkpath)

node.addObject("toto", position=position.linkpath)  # makes a link without path conversion
node.addObject("toto", position=str(position.linkpath))  # makes a link with path conversion
node.addObject("toto", position=position.path)  # makes a link with path conversion

In general I think it is better to learnability and API understanding to have a single way of doing things instead of two. Otherwise people needs acquire the knowledge that linkpath and path are not equivalent.

@hugtalbot
Copy link
Contributor

Principle ok, but we need to create an issue keeping track of the change

@hugtalbot
Copy link
Contributor

PR has some conflicts poke @damienmarchal

@damienmarchal
Copy link
Contributor Author

Hi @hugtalbot, thank for pointing I will look at that after the release.

@hugtalbot
Copy link
Contributor

I like your proposal:

node.addObject("toto", position=position.linkpath)  # makes a link without path conversion
node.addObject("toto", position=str(position.linkpath))  # makes a link with path conversion
node.addObject("toto", position=position.path)  # makes a link with path conversion

with a deprecation of .getLinkPath()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pr: status ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants