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

precollision event inconsistencies between Actor hook and ColliderComponent event #3098

Closed
mattjennings opened this issue Jun 16, 2024 · 3 comments · Fixed by #3238
Closed
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior stale This issue or PR has not had any activity recently

Comments

@mattjennings
Copy link
Contributor

mattjennings commented Jun 16, 2024

The value of other in the precollision event is either a collider or the actor, depending on where you're listening to the event from.

Actor's onPreCollisionResolve is a collider

ColliderComponent event is the entity, which is wiring from what the System emits

I would suggest keeping them always as colliders.


Tangentially (this could be its own issue but I figured i'd throw it in here), I wonder if lifecycle methods like onXYZ() on actors should always match the event payload. In other words 1 param as the event. Sometimes it's a pain when you want to move an event handler as a .on() handler to/from an Actor lifecycle method because one is multiple params whereas the other is an event object.

@eonarheim eonarheim added the bug This issue describes undesirable, incorrect, or unexpected behavior label Jun 20, 2024
@eonarheim
Copy link
Member

I agree, I think keeping them as the Collider would be less confusing

Tangentially (this could be its own issue but I figured i'd throw it in here), I wonder if lifecycle methods like onXYZ() on actors should always match the event payload. In other words 1 param as the event. Sometimes it's a pain when you want to move an event handler as a .on() handler to/from an Actor lifecycle method because one is multiple params whereas the other is an event object.

It definitely feel like it should match... I'd support changes to make this so

@mattjennings
Copy link
Contributor Author

cool, I've split that out into a separate issue then! #3106

Copy link

This issue hasn't had any recent activity lately and is being marked as stale automatically.

@github-actions github-actions bot added the stale This issue or PR has not had any activity recently label Aug 20, 2024
eonarheim added a commit that referenced this issue Oct 23, 2024
* fix: consistent events

* fix tests

* fix lint

* fix sandbox

* fix docs

* fix docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior stale This issue or PR has not had any activity recently
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants