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

CollisionGroup not updating value #346

Open
ScheiklP opened this issue May 9, 2023 · 14 comments
Open

CollisionGroup not updating value #346

ScheiklP opened this issue May 9, 2023 · 14 comments

Comments

@ScheiklP
Copy link
Collaborator

ScheiklP commented May 9, 2023

Hi! :)

I am trying to change the collision group of a collision model during runtime.
However, the values are not updated correctly, and the .array() method returns weird values.

Example to reproduce:

import numpy as np
import Sofa
import Sofa.Core
import Sofa.Simulation


class SimulationHandler:
    def __init__(self):
        self.root_node = Sofa.Core.Node("root")
        self.nodes = createScene(self.root_node)
        Sofa.Simulation.init(self.root_node)

    def step(self):
        Sofa.Simulation.animate(self.root_node, self.root_node.getDt())


def createScene(root):
    plugins = [
        "Sofa.Component.Collision.Detection.Algorithm",
        "Sofa.Component.Collision.Detection.Intersection",
    ]
    for plugin in plugins:
        root.addObject("RequiredPlugin", pluginName=plugin, name=plugin)

    root.addObject("DefaultAnimationLoop")
    root.addObject("DefaultPipeline")
    root.addObject("BruteForceBroadPhase")
    root.addObject("BVHNarrowPhase")
    root.addObject("DefaultContactManager")
    root.addObject(
        "LocalMinDistance",
        alarmDistance=5.0,
        contactDistance=0.5,
    )

    node_1 = root.addChild("child_1")
    node_1.addObject("MechanicalObject", position=[0, 0, 0] * 5)
    node_1.addObject("PointCollisionModel", group=0)

    node_2 = root.addChild("child_2")
    node_2.addObject("MechanicalObject", position=[1, 1, 1] * 5)
    node_2.addObject("PointCollisionModel", group=1)

    return node_1, node_2


if __name__ == "__main__":
    simulation = SimulationHandler()

    simulation.step()
    print(f"First step: {simulation.nodes[0].PointCollisionModel.group.value=}")
    print(f"First step: {simulation.nodes[1].PointCollisionModel.group.value=}")
    print(f"Array: {simulation.nodes[0].PointCollisionModel.group.array()=}")
    print(f"Array: {simulation.nodes[1].PointCollisionModel.group.array()=}")
    print("-----")
    simulation.nodes[0].PointCollisionModel.group.value[:] = np.array([8])
    simulation.nodes[1].PointCollisionModel.group.value[:] = np.array([9])
    print(f"After setting: {simulation.nodes[0].PointCollisionModel.group.value=}")
    print(f"After setting: {simulation.nodes[1].PointCollisionModel.group.value=}")
    print(f"Array: {simulation.nodes[0].PointCollisionModel.group.array()=}")
    print(f"Array: {simulation.nodes[1].PointCollisionModel.group.array()=}")
    simulation.step()
    print("-----")
    print(f"Second step: {simulation.nodes[0].PointCollisionModel.group.value=}")
    print(f"Second step: {simulation.nodes[1].PointCollisionModel.group.value=}")
    print(f"Array: {simulation.nodes[0].PointCollisionModel.group.array()=}")
    print(f"Array: {simulation.nodes[1].PointCollisionModel.group.array()=}")

Output:

First step: simulation.nodes[0].PointCollisionModel.group.value=[[0]]
First step: simulation.nodes[1].PointCollisionModel.group.value=[[1]]
Array: simulation.nodes[0].PointCollisionModel.group.array()=array([1073741824], dtype=int32)
Array: simulation.nodes[1].PointCollisionModel.group.array()=array([1073741824], dtype=int32)
-----
After setting: simulation.nodes[0].PointCollisionModel.group.value=[[0]]
After setting: simulation.nodes[1].PointCollisionModel.group.value=[[1]]
Array: simulation.nodes[0].PointCollisionModel.group.array()=array([1073741824], dtype=int32)
Array: simulation.nodes[1].PointCollisionModel.group.array()=array([1073741824], dtype=int32)
-----
Second step: simulation.nodes[0].PointCollisionModel.group.value=[[0]]
Second step: simulation.nodes[1].PointCollisionModel.group.value=[[1]]
Array: simulation.nodes[0].PointCollisionModel.group.array()=array([1073741824], dtype=int32)
Array: simulation.nodes[1].PointCollisionModel.group.array()=array([1073741824], dtype=int32)

Any ideas what might be wrong here?

Sofa commit: sofa-framework/sofa@9a0d4b9
SP3 commit: 5a73716

Cheers,
Paul

@hugtalbot
Copy link
Contributor

hugtalbot commented May 9, 2023

Hey @ScheiklP

I tested a very simple xml scenario where I set in the XML the group datafield of the CollisionModels and then change one and it does take the change into account (all initially group=0 and then I change for one object group=1 and this object goes through the other ones)

Maybe something related to the binding between the python array and the C++ Data which is a std::set<int>
@damienmarchal could maybe help here..

Thanks for reporting it anyway @ScheiklP 👍

@hugtalbot
Copy link
Contributor

Sorry my bad, it's indeed not working 👍

@ScheiklP
Copy link
Collaborator Author

ScheiklP commented May 9, 2023

@hugtalbot So it's more a Sofa than a SofaPython3 issue? Should I open another issue there?

@hugtalbot
Copy link
Contributor

Yes 👍

@hugtalbot
Copy link
Contributor

hugtalbot commented May 9, 2023

My first feeling would be that this is due to the 2-step broad-narrow phases : the data group concerns both but when modifying the group of the CollisionModel of your object, it's actually used for the narrow phase (Proximity) while the group information should be propagated to all phases. In addition I guess that this group even gets overwritten (in the GUI after changing it, and reopening it, the value is the initial one)

Let's investigate

@alxbilger
Copy link
Contributor

I did

self.assertEqual(node.PointCollisionModel.group.value, [[0]])
node.PointCollisionModel.group = [8]
self.assertEqual(node.PointCollisionModel.group.value, [[8]])

and the result is

self.assertEqual(node.PointCollisionModel.group.value, [[8]])
AssertionError: Lists differ: [[0], [8]] != [[8]]

It seems that setting a value in a set insert the new value, but does not remove the old one. I think it is due to https://github.com/sofa-framework/sofa/blob/918cd66008f586575c92c1e068f5c267a952b936/Sofa/framework/DefaultType/src/sofa/defaulttype/typeinfo/models/SetTypeInfo.h#L113.

I think this discussion should involve @damienmarchal.

@alxbilger
Copy link
Contributor

Here is the unit test:

# coding: utf8

import Sofa.Core
import Sofa.Components
import unittest
import numpy as np

class Test(unittest.TestCase):
    def test_setting_collision_group(self):
        root = Sofa.Core.Node("rootNode")
        plugins = [
            "Sofa.Component.Collision.Detection.Algorithm",
            "Sofa.Component.Collision.Detection.Intersection",
        ]
        for plugin in plugins:
            root.addObject("RequiredPlugin", pluginName=plugin, name=plugin)

        root.addObject("DefaultAnimationLoop")
        root.addObject("DefaultPipeline")
        root.addObject("BruteForceBroadPhase")
        root.addObject("BVHNarrowPhase")
        root.addObject("DefaultContactManager")
        root.addObject("LocalMinDistance", alarmDistance=5.0, contactDistance=0.5)

        node = root.addChild("child")
        node.addObject("MechanicalObject", position=[0, 0, 0] * 5)
        node.addObject("PointCollisionModel", group=0)

        Sofa.Simulation.init(root)
        Sofa.Simulation.animate(root, root.getDt())

        self.assertEqual(node.PointCollisionModel.group.value, [[0]])
        node.PointCollisionModel.group = [8]
        self.assertEqual(node.PointCollisionModel.group.value, [[8]])

@damienmarchal
Copy link
Contributor

Thanks for having added me in this nicely documented issues with code to code & paste for testing things.

@damienmarchal
Copy link
Contributor

damienmarchal commented May 10, 2023

Ok, so after quick investigation, there are several issues related to how sets are exposed to python related to what @alxbilger pointed. The real problem is that SetTypeinfo is "hijacking" the typeinfo API for indexable container. This is very visible in the SetTypeInfo::setSize() that actually clears the set as well as using setValue(..., index, ) which is ignoring the index and insert the data.

The consequence of that is that sets are considered as an indexable container by the python binding but as they are not really fullfilling what could be expected from an indexable container things goes badly wrong.

eg:

  • when we get the value from a set it get interpreted as a two dimmensional array when converted to python. This is why "value" returns [[0],[1],[2]] instead of [0,1,2] which is a consistency issue when setting the value which expect [1,2,3] and not [[1],[2],[3]]

  • when we set the value from python. The codepath is the one for a indexed container, this code path is using setSize() to initialize data only if the current set size and the new ones mis-matches (which is not the case in the provided example). The consequence is that setSize() is not called at all, which is perfectly ok for a resizable container but not for a set.

Quick fix can be done in the python binding by calling setSize() everytime we do setValue, this will force the set to be cleared, this fix only solve the reported issue but not the other problems (the conversion when we get the data).

A much clean way of fixing that is to add complete support for "Unique Key containers" the typeinfo system with specific dedicated API. This probably means adding feature in AbstractTypeInfo.

@hugtalbot
Copy link
Contributor

Thanks for the feedback @damienmarchal

@ScheiklP
Copy link
Collaborator Author

@damienmarchal Thanks a lot! :)
Could you point me to the relevant code for the quick fix? 😅

@damienmarchal
Copy link
Contributor

damienmarchal commented May 11, 2023

@damienmarchal Thanks a lot! :) Could you point me to the relevant code for the quick fix? sweat_smile

In PythonFactory.cpp you need to comment the pointed line...

template<class DestType>
void copyFromListOf(BaseData& d, const AbstractTypeInfo& nfo, const py::list& l)
{
    /// Check if the data is a single dimmension or not.
    py::buffer_info dstinfo = toBufferInfo(d);

    if(dstinfo.ndim>2)
        throw py::index_error("Invalid number of dimension only 1 or 2 dimensions are supported).");

    if(dstinfo.ndim==1)
    {
        void* ptr = d.beginEditVoidPtr();
        if( size_t(dstinfo.shape[0]) != l.size())        // THIS IS THE LINE TO COMMENT TO FORCE CLEARING of a set<- 
            nfo.setSize(ptr, l.size());

        for(size_t i=0;i<l.size();++i)
        {
            copyFromListOf<DestType>(nfo, ptr, i, l[i]);
        }
        d.endEditVoidPtr();
        return;
    }
    /// ...
}

NB: I tried to fix that the right way... but working in TypeInfo.h is such a source of pain because of its misdesign that I will probably give-up soon.

@hugtalbot
Copy link
Contributor

Did this quick-dirty fix solve your issue @ScheiklP ?

@ScheiklP
Copy link
Collaborator Author

I did not try it yet. I was hoping for a miracle in sofa-framework/sofa#3851 :D

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

No branches or pull requests

4 participants