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

Make most properties use conventional syntax to fix static typing #273

Merged
merged 1 commit into from
Feb 22, 2025

Conversation

aatle
Copy link

@aatle aatle commented Feb 21, 2025

Fixes #272

Turn most properties that use functional syntax (x = property(_get, _set, doc="")) into normal properties using the decorator syntax.
Additionally, a few minor type annotation fixes on some properties.

@viblo
Copy link
Owner

viblo commented Feb 21, 2025

Thanks!
I see you left out some properties in collision_handler.py, was that on purpose?
(the other ones returning Vec2d I assume you skipped on purpose because of the type checking issue)

@aatle
Copy link
Author

aatle commented Feb 22, 2025

I left out some properties in collision_handler.py for the same reason that Vec2d properties were left out: their setter type differs from the getter type.
In this case, the getter may return None but the setter may not set None, for the callbacks.

If possible and easy, it would be nice to fix this inconsistency. There's two ways that I can think of:

  1. Allow setting None, if possible.
  2. If there are no performance nor implementation issues, use actual default callbacks instead of indicating default with None. Note that the default callback would be convenient to have for users because it would allow them to call wildcard collision handlers from their custom callback. (also there is still a fragment in the docs referring to the removed functionality of calling wildcards, at Space.add_wildcard_collision_handler).

@viblo viblo merged commit b7b298d into viblo:pymunk7 Feb 22, 2025
5 checks passed
@viblo
Copy link
Owner

viblo commented Feb 22, 2025

The collision callbacks are so basic (since their implementation will be just return True or return ), so I dont think a default callback will be of use outside of setting it to "reset" to handler.

On the other hand, that is how it works in velocity_func and position_func on the body. If the user pass in the default one I have special code to replace the callback with a pure Chipmunk default callback so there's no call into python and no performance hit in those cases.

For consistency the collision callbacks could work in the same way.. In the end the effect is the same as allow setting None, but for consistency and clarity it might be better to handle it as the position/velocity callbacks.

@aatle
Copy link
Author

aatle commented Feb 22, 2025

I looked into this more, and I now understand the problem I was trying to get at.

The CollisionHandlers returned by specific Space.add_collision_handler() and default Space.add_default_collision_handler() have built-in callbacks that call the wildcard handlers and if begin or pre_solve, && their results together.

The CollisionHandlers returned by Space.add_wilcard_handler() have built-in callbacks that do nothing (if applicable, return True).

These are two different sets of default callbacks, but CollisionHandler marks all of them as None.
Additionally, for the specific and default handlers, the user currently has no way to invoke the built-in behavior of calling wildcard handlers. This means if they use Space.add_collision_handler() and Space.add_default_collision_handler(), the wildcard handler functionality won't work for those collision types.

Therefore, to align more closely with the chipmunk implementation, and be more pythonic, it might be good to have real callbacks matching the callbacks at:
https://github.com/slembcke/Chipmunk2D/blob/d0239ef4599b3688a5a336373f7d0a68426414ba/src/cpSpace.c#L65-L103
(would be a breaking change, of course)

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

Successfully merging this pull request may close these issues.

2 participants