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

Body.copy() with multiple shapes attached to a body #239

Open
GuillaumeBroggi opened this issue Nov 21, 2023 · 4 comments
Open

Body.copy() with multiple shapes attached to a body #239

GuillaumeBroggi opened this issue Nov 21, 2023 · 4 comments

Comments

@GuillaumeBroggi
Copy link

The copy of a body attached to multiple shapes raises an AssertionError: The shape's body must be added to the space before (or at the same time) as the shape. when following the usage shown in the examples.

The only workaround I have found is to get a new body and only copy the shapes (see below).

import pymunk

s = pymunk.Space()
b1 = pymunk.Body(1, 1)
b1.position = 1, 2

shape1 = pymunk.Circle(b1, 4)
shape2 = pymunk.Circle(b1, 6)
s.add(b1, shape1, shape2)

# Work, with a new object
b2 = pymunk.Body()
shape_copies = []
for shape in b1.shapes:
    shape_copies.append(shape.copy())
    shape_copies[-1].body = b2

s.add(*shape_copies, b2)

# Do not work
b3 = b1.copy()
shape_copies = []
for shape in b1.shapes:
    shape_copies.append(shape.copy())

s.add(*shape_copies, b3)

# Do not work
shape_copies = []
for shape in b1.shapes:
    shape_copies.append(shape.copy())

s.add(*shape_copies, shape_copies[0].body)
@viblo
Copy link
Owner

viblo commented Nov 21, 2023

Oh.. It seems to work properly if the whole space is copied. But not if the shape or body is copied separately..

In the your last case it looks like the issue is that the two shapes copies have different bodies, but only one of them is added to the space which is why it breaks.

I will have to investigate a bit

@GuillaumeBroggi
Copy link
Author

In the last case the shapes are attached to the same body, so the I was (maybe naively :) ) expecting that the shape copies would reference the same body object.

Instead, it seems that the copy is done sequentially and each shape is attached to its own copy of the initial body:

print(hex(id(b1)))

shape_copies = []
for shape in b1.shapes:
    shape_copies.append(shape.copy())
    print(hex(id(shape_copies[-1].body)))
    
s.add(*shape_copies, shape_copies[0].body)

0x7fa74d0fdf50 # b1
0x7fa74cde4c90 # first copy of b1
0x7fa74cde5290 # second copy of b1

Thus both body copies should be added to the space, but the shapes will not be part of the same body anymore. A workaround could be to replace the body of the additional shape copies with the one from the first shape copy, as below, but it means that we may be doing a lot of body copying for nothing:

shape_copies = []
for shape in b1.shapes:
    shape_copies.append(shape.copy())

for shape in shape_copies[1:]:
    shape.body = shape_copies[0].body

s.add(*shape_copies, shape_copies[0].body)

@viblo
Copy link
Owner

viblo commented Nov 23, 2023

Thinking about this some more, Im not sure if this is a bug or feature :)

Copy a space is "easy", or at least the expectation of what should happen is easy (except for maybe callback functions and other special cases). Everything inside should be copied.

But for single bodies or shapes Im not sure:

  • A body does not have a direct reference to the shapes to avoid circular dependency. It only has a weakref to it for convenience (its the shape that is attached to a body and not other way around). So if its copied, its natural that only the body is copied without the shapes that are attached to it. The same applies to constraints the body is attached to.
  • A shape has a body, when its copied then the body is also copied over.
  • But two shapes with the same body.. ? When the first shape is copied over body is also copied. But when second shape is copied, then its very messy if it should attach itself to the first copied shapes' body (since it would mean that some state would be needed to keep track of all copies and originals). And there are other corner cases here, like if you modify the body in between..
  • Neither shapes nor bodies that are copied will have the space set on the copy.
  • And what about the complex case when there's constraints as well.

I guess the copy could follow all references between bodies, shapes and constraints so if you copy one thing then everything else is included. However, this is quite complicated since creation of the copies has to happen in the right order or it will break..

Given all this Im not sure I would have an API for it at all except to copy space unless it didnt already exist..

@GuillaumeBroggi
Copy link
Author

From a user perspective, it would make sense to have a "ready to use" copied object, with all referenced objects.

But I see that it would be very challenging from the API perspective, so I guess it's not an idea to consider 😄

As an alternative, could it make sense to have an option to only copy the shape (and not the referenced body for instance) so no unnecessary copying happens when working with multiple shapes? Assuming that these body extra copies have an impact on performances.

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

2 participants