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

[Binding/Sofa.Core] PointSetTopologyModifier #290

Merged
merged 3 commits into from
Nov 3, 2022

Conversation

ScheiklP
Copy link
Collaborator

I want to add bindings for PointSetTopologyModifier's addPoints and removePoints in order to change the size of a PointSetTopologyContainer that are propagated to components that are linked with the container (e.g. MechanicalObject, RestShapeSpringsForceField, FixedConstraint, ...).

Work in Progress, because the binding does not work as expected and I could use some help on that.
I added an example in https://github.com/ScheiklP/SofaPython3/blob/point_topology_modifier/examples/pointSetTopologyModifier.py

On first call, addPoints works as expected, every call after that does not. removePoints works well, but validity of point to be removed is not checked yet. Should renumber also be called?

topology-2022-07-24_16.24.15.mp4

Also if I try to open the MechanicalObject from within the GUI after modifying the topology, I get a SegFault.

…nts works as expected, every call after that does not. Remove points works well, but validity of point to be removed is not checked yet.
@ScheiklP ScheiklP marked this pull request as draft July 24, 2022 14:48
@hugtalbot
Copy link
Contributor

Maybe the @epernod 's expert 👀 would be useful here :)

@epernod
Copy link
Contributor

epernod commented Jul 29, 2022

Hi,

could you develop what you mean by: every call after that does not
just give me an example of what is not working vs expected behavior.

Regarding removing of points, the process of renumbering is the same as I explained for removing tetrahedron in this issue: sofa-framework/sofa#3156

@ScheiklP
Copy link
Collaborator Author

Case A:

  1. I call the function to add 10 points -> 10 points are added (10 more visualized points pop up)
  2. I call the function to add 10 points again -> No additional visual points pop up. If the MO is already open in the GUI, the nbPoints increases, but I cannot see them in the state. If the MO is NOT already open in the GUI, and I try to open it -> segfault

So for 2. I would expect 10 more points to show up :D

Case B:

  1. I call the function to add 10 points -> 10 points are added (10 more visualized points pop up)
    [0, 1, ...., 11]
  2. I call the function to delete index 0 -> The left most point disappears
  3. I call the function to delete index 0 -> The right most point disappears
  4. I call the function to delete index 0 -> The right most point disappears
  5. I call the function to add 10 points -> Only one point is added (the most left one, that disappeared in 2.)

Which is particularly weird, because I "reposition" them after trying to add them

            with self.state.position.writeable() as state:
                for i in range(len(state)):
                    state[i] = np.array([i, 0, 0])

@fredroy
Copy link
Contributor

fredroy commented Aug 31, 2022

Still WIP ?

@ScheiklP
Copy link
Collaborator Author

Still WIP ?

Yes, because I still do not know where the observed behavior comes from.

@ScheiklP
Copy link
Collaborator Author

ScheiklP commented Sep 4, 2022

Ok I have further investigated the problem a bit, and I have a feeling that the changes are not properly passed to the MechanicalObject.
If I add the 10 points, the size of position.writeable() correctly changes to 12. If I then add or remove further points, the size remains at 12.

@damienmarchal
Copy link
Contributor

During the sofadevmeeting we look at this PR. any update is it still in WIP ?

@ScheiklP
Copy link
Collaborator Author

During the sofadevmeeting we look at this PR. any update is it still in WIP ?

Hi @damienmarchal
There is still the bug with the incorrectly updated MechanicalObject.
Sadly I know too little about the event mechanism for topology changes to start investigating the problem properly.

@adagolodjo
Copy link
Contributor

During the sofadevmeeting we look at this PR. any update is it still in WIP ?

Hi @damienmarchal There is still the bug with the incorrectly updated MechanicalObject. Sadly I know too little about the event mechanism for topology changes to start investigating the problem properly.

Hello @damienmarchal,
Just a quick nod to this topic again, if you have some time.
Thanks !

@damienmarchal
Copy link
Contributor

Hi all,

After a quick investigation with @younesssss i would say the problem is in the c++ part. And @epernod is clearly the one to ask for insight, on my side I have zero knowledge on the topology change.

@ScheiklP
Copy link
Collaborator Author

ScheiklP commented Oct 19, 2022

Hi @damienmarchal @hugtalbot ,
together with @epernod we found the underlying issue.

When trying to write into MechanicalObject.position, with writeable() or __setitem__, SofaPython3 checks if
the data is dirty or not in memcache.
For some reason, the d->isDirty() check always evaluates to false.
So after calling the function the first time, d is in memcache and the not-updated version of the array is used.
This is why len(MechanicalObject.position.writeable() is incorrect.

https://github.com/sofa-framework/SofaPython3/blob/master/Plugin/src/SofaPython3/DataHelper.cpp#L430-L445

py::array getPythonArrayFor(BaseData* d)
{
    auto& memcache = getObjectCache();
    if(d->isDirty() || memcache.find(d) == memcache.end())
    {
        auto capsule = py::capsule(new Base::SPtr(d->getOwner()), [](void*p){ delete static_cast<Base::SPtr*>(p); } );

        py::buffer_info ninfo = toBufferInfo(*d);
        py::array a(pybind11::dtype(ninfo), ninfo.shape,
                    ninfo.strides, ninfo.ptr, capsule);

        memcache[d] = a;
        return a;
    }
    return memcache[d];
}

Since the binding itself is functional, I would like to merge it, and then open another issue to address the actual problem.

  • Long term, by making sure that d->isDirty() correctly evaluates to true.
  • Short term, by checking if the shapes of d and memcache[d] are the same.

Cheers,
Paul

@ScheiklP ScheiklP changed the title [Binding/Sofa.Core] PointSetTopologyModifier WIP [Binding/Sofa.Core] PointSetTopologyModifier Oct 19, 2022
@hugtalbot
Copy link
Contributor

Hi @damienmarchal could you have you 🐍 python eyes 👁️ on above Paul's comment?

Copy link
Contributor

@epernod epernod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ScheiklP for me this PR is ready as the problem of updateIfDirty is a more general problem not linked to your PR. An issue has been created to keep track this.
Could you "undraft" your PR so we can merge it.

@ScheiklP ScheiklP marked this pull request as ready for review November 2, 2022 15:41
@fredroy fredroy merged commit 15bc972 into sofa-framework:master Nov 3, 2022
@adagolodjo
Copy link
Contributor

Hello @damienmarchal,
Did you push the latest changes you made on this thread? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants