Is there a smarter way to handle the Optional type for shape.body?

[viblo]

Quite often you iterate over the shapes of the space, or maybe you get a shape out from a arbiter, and then you need to fetch the body of the shape. In these cases you know for sure that the body is set, because a shape cannot be added to a space unless it has a body which is also added to the space. But the type checked doesn't know this and will warn that it might be None.

space = pymunk.Space()
for shape in space.shapes
    position = shape.body.position  # type error here since I didnt assert body is not None.

Is there some smarter way to handle this to reduce the useless asserts?

[aatle]

From my experience with type hinting, for the given code there is not a way to avoid one of the following:

• Check (error assertion or if statement)
• typing.cast()
• # type: ignore

If I encounter an annoying optional, I either try to replace it with raising an error, provide/define another function that does the narrowing, replace with an iterable, or rewrite the code to avoid it. From these options, I see solutions to your examples.

1. Space shape bodies: Reverse the access dependency. This makes more sense as the body intuitively own the shapes. It is also more flexible.

space = pymunk.Space()
for body in space.bodies:
    position = body.position
    for shape in body.shapes:
        ...

2. Arbiter bodies: Provide direct access, 'shortcut' (new API). In fact, in an arbiter proxy in my project, I provide the additional properties .bodies and .collision_types (and project-specific .entities).

class Arbiter:
    ...
    @property
    def bodies(self) -> Tuple["Body", "Body"]: ...

(side note: in my code, for performance reasons, the arbiter proxy caches the shapes because they are accessed at least once and are used in additional properties)

[aatle]

This reminds me of an annoyance I've had: the QueryInfo types all have optional .shape, which seems unnecessary given that most of the methods that provide it either return an iterable of QueryInfo or an optional QueryInfo.
It also isn't clear what the collision data of the QueryInfo is when the shape is None (indicating no collision).

[viblo]

This reminds me of an annoyance I've had: the QueryInfo types all have optional .shape, which seems unnecessary given that most of the methods that provide it either return an iterable of QueryInfo or an optional QueryInfo. It also isn't clear what the collision data of the QueryInfo is when the shape is None (indicating no collision).

Ah true! Only one case did actually use the None feature, Shape.segment_query. However, I do not see a good use of that. I removed it and made the .shape non-optional in all 3 of the query info objects.

[viblo]

In addition to the Arbiter case there's at least two more cases I can think of:

1. When you get out a shape from a space query. Then you have the same shape.body issue as in the Arbiter. With the above improvement at least there's no need to assert the shape is not None for these cases, but the body could still be.. And it wouldn't easily help to add another body shortcut, since the QueryInfo object can be returned from both query on the space (which mean the shape will have a body), and from query with a shape (which mean the shape might not have a body).

2. If you need to do shape.space or body.space. On the other hand I cant think of a good example when these properties are useful, maybe no-one uses them...?

Im a little hesitant to add a body shortcut to Arbiter, since it sort of goes against only-one-way-to-do-it. On the other hand Pymunk already has a number of shortcuts like the space property just above.

[viblo]

Answering myself about the QueryInfo objects: Maybe the Shape.point_query and Shape.segment_query could return a tuple of the result instead of an object, or make a new object just for this. The currently returned objects contain the shape property which is useless in this case anyway, if you query a shape for something you already know the shape.

[aatle]

I would avoid returning heterogenous raw tuples, NamedTuple is preferable.
While more NamedTuples can be defined for separating shape from space queries, I think this would not be good as more classes have to be added to the docs. However, there are already a ton of small, struct-like classes in the docs I guess.
A few comments about this:

• ContactPoint and ContactPointSet could possibly be NamedTuples, for consistency and to make it immutable.
  • Additionally, ContactPointSet.points could be Tuple[ContactPoint, ...] instead of List[ContactPoint]
• The NamedTuple classes in the auto-generated docs show __add__ and __mul__ tuple methods. If possible, these should be removed to improve clarity and conciseness.
• If the classes in the docs could be grouped by purpose and importance, rather than sphinx default alphabetical sorting, this could greatly increase clarity and reduce the impact of tiny info classes. I'll also mention that navigation can be cumbersome because the classes table of contents is always at the top and the page is very large, leading me to use ctrl+f to jump to classes.

If not adding a new query info, then possibly a body property that raises AttributeError if it is None could be added as it is unlikely that it would be accessed as None. But this doesn't really seem right. An optional body property could also be added anyway, in the interest of increasing convenience.

---

I do think it is beneficial to add arbiter.bodies though. From the user's point of view, they don't care if the arbiter was only given shapes, that each belong to a body. To them, an arbiter represents a collision that intuitively involves two bodies, with the bodies usually being more useful than the shapes.

shape.space and body.space are probably fine to keep as optional, as that is more useful, accessing it is quite rare (because most programs use only one space, it is strange to access from a shape or body), and easily avoided with better design.

As always, the impact of these shortcuts should be weighed against the extra convenience of avoiding an optional check per access.

[viblo]

I would avoid returning heterogenous raw tuples, NamedTuple is preferable. While more NamedTuples can be defined for separating shape from space queries, I think this would not be good as more classes have to be added to the docs. However, there are already a ton of small, struct-like classes in the docs I guess. A few comments about this:

* `ContactPoint` and `ContactPointSet` could possibly be `NamedTuple`s, for consistency and to make it immutable.

A, there's one issue, you are allowed to update the values. This is why they are not NamedTuples. I guess this is not be most well known feature, and its not very well documented, but it should work.

  * Additionally, `ContactPointSet.points` could be `Tuple[ContactPoint, ...]` instead of `List[ContactPoint]`

Ah, maybe this would be a good idea. While you are allowed to change properties on the points, you are not allowed to change the number of points, so a tuple could make it clearer.

* The `NamedTuple` classes in the auto-generated docs show `__add__` and `__mul__` tuple methods. If possible, these should be removed to improve clarity and conciseness.

A, yes, should be fixed.

* If the classes in the docs could be grouped by purpose and importance, rather than sphinx default alphabetical sorting, this could greatly increase clarity and reduce the impact of tiny info classes. I'll also mention that navigation can be cumbersome because the classes table of contents is always at the top and the page is very large, leading me to use ctrl+f to jump to classes.

Good comment. In general I dont think ctrl-f to look around is so bad, but maybe its a good idea to break it up more now. It has had this design for a very long time, and at that time there were much less docs.

If not adding a new query info, then possibly a body property that raises AttributeError if it is None could be added as it is unlikely that it would be accessed as None. But this doesn't really seem right. An optional body property could also be added anyway, in the interest of increasing convenience.

I do think it is beneficial to add arbiter.bodies though. From the user's point of view, they don't care if the arbiter was only given shapes, that each belong to a body. To them, an arbiter represents a collision that intuitively involves two bodies, with the bodies usually being more useful than the shapes.

shape.space and body.space are probably fine to keep as optional, as that is more useful, accessing it is quite rare (because most programs use only one space, it is strange to access from a shape or body), and easily avoided with better design.

As always, the impact of these shortcuts should be weighed against the extra convenience of avoiding an optional check per access.

Yeah, I think I will add it, it does make sense as you note.

ps.
Right now (from the shape body dependency reversal I believe) Im looking at a GC seg fault that happens on some python/os combinations in github actions that I want to fix before adding / updating anything else. Unfortunately these errors are very troublesome to debug...

[aatle]

A, there's one issue, you are allowed to update the values. This is why they are not NamedTuples. I guess this is not be most well known feature, and its not very well documented, but it should work.

Hm, this still confuses me. I see that Arbiter.contact_set_point has a setter, but this won't be invoked if the values are updated by setting attributes on the original. Also it's possible to update contact point set everywhere where it is returned?

Right now (from the shape body dependency reversal I believe) Im looking at a GC seg fault that happens on some python/os combinations in github actions that I want to fix before adding / updating anything else. Unfortunately these errors are very troublesome to debug...

That sounds awful.
I see in the commit history that the tests start failing after df54445, or is that unrelated? If it is from the shape body dependency reversal then maybe weakref.ref's callback optional argument could help debug?

[viblo]

A, there's one issue, you are allowed to update the values. This is why they are not NamedTuples. I guess this is not be most well known feature, and its not very well documented, but it should work.

Hm, this still confuses me. I see that Arbiter.contact_set_point has a setter, but this won't be invoked if the values are updated by setting attributes on the original. Also it's possible to update contact point set everywhere where it is returned?

Yeah, cant say its a perfect API here. Underneath it wont matter if you make a new point set or modify the one you got out of the arbiter. In the end it will copy over the values to an internal struct anyway. But it is easier to to reuse the existing one than to make a new, since most likely you want to only modify some parts of it (you are not even allowed to modify everything, i.e. the number of points must be the same).

The reason for this style is that its how it works in Chipmunk, you get a struct out, and you can pass a struct in. I'm open for adjustments here, but its not clear to me how. Since you get a copy it will live as long as you like and you don't need to think about lifetime like with the Arbiter. And you are in control when to assign its values back and so on.

Right now (from the shape body dependency reversal I believe) Im looking at a GC seg fault that happens on some python/os combinations in github actions that I want to fix before adding / updating anything else. Unfortunately these errors are very troublesome to debug...

That sounds awful. I see in the commit history that the tests start failing after df54445, or is that unrelated? If it is from the shape body dependency reversal then maybe weakref.ref's callback optional argument could help debug?

Yeah, its very annoying and time consuming to debug this. If it happened on all os/pythons it would be so much easier, and if it was not GC-triggered seg faults in the c code it would also be much easier..

The current one is a good example:
In the commit you linked cp312-musllinux_x86_64 failed. And it fails on the GC done when Python shuts down after all the tests are reported as passed.
But in the latest commit, then that platform is passed, instead its cp313-musllinux_x86_64 that fails. And it fails on a test (testDeleteSpaceWithObjects) and not on the final GC cleanup.
And in both cases its musl and not the normal glibc, so I cant just reproduce on my standard WSL Ubuntu..

[viblo]

Closing this one now since a number of changes were committed.