jcornaz / heron

[DISCONTINUED] An ergonomic physics API for bevy games
MIT License
293 stars 44 forks source link

feat!: add normals to `CollisionData` in events #189

Closed tqwewe closed 2 years ago

tqwewe commented 2 years ago

Adds normal data from ContactManifoldData::normal to the CollisionData struct.

Benefits

Downsides


Fixes https://github.com/jcornaz/heron/issues/188

jcornaz commented 2 years ago

I had to remove Copy from CollisionData due to the normals being in a Vec. We could just provide the first normal Vec2 as an Option, but I'm not sure whats the best option for this.

Arf. That's a quite big downside indeed. But the benefits are high too. I'll try to figure out how to move forward (which may simply be: releasing a breaking change).

jcornaz commented 2 years ago

Maybe only provide the first normal as an option as you suggested. At least it doesn't force us to make a breaking change, and it should be enough for most use-cases. It may even make it easier to use actually.

Even if we end up changing our mind and break the API later to introduce a Vec, then we would have at least deferred breaking the API until we really needed it.

What do you think?

tqwewe commented 2 years ago

I think it should cover most cases. If someone creates an issue for it, then it could be easily changed, or maybe just released in the next major/breaking release.

I'll go ahead and update it to Option for now

tqwewe commented 2 years ago

Although, it seems like Vec2 does not implement Eq... so even as Option<Vec2>, it will still be a breaking change regardless.

jcornaz commented 2 years ago

Although, it seems like Vec2 does not implement Eq... so even as Option, it will still be a breaking change regardless.

Arf... I'll think more about it. Don't change anything yet.

tversteeg commented 2 years ago

Although, it seems like Vec2 does not implement Eq... so even as Option<Vec2>, it will still be a breaking change regardless.

Can't you manually implement Eq and ignore the Vec2?

jcornaz commented 2 years ago

Can't you manually implement Eq and ignore the Vec2?

That would be an incorrect equal implementation IMO. If two event datas don't have the same normal, then they are not equal.

tversteeg commented 2 years ago

Can't you manually implement Eq and ignore the Vec2?

That would be an incorrect equal implementation IMO. If two event datas don't have the same normal, then they are not equal.

You could also compare the normals in the eq manually.

jcornaz commented 2 years ago

You could also compare the normals in the eq manually.

Vec2 only implements PartialEq, because in some cases a vector is not equal to itself. So we can only at most implement PartialEq. Implementing Eq is not possible, even manually, because we still have to account for the fact that Vec2 only has a partial equivalence relation.

tqwewe commented 2 years ago

Out of curiosity, why wouldn't Vec2 already implement Eq? In what cases would a vec not equal itself?

jcornaz commented 2 years ago

In what cases would a vec not equal itself?

Vec2::NAN is not equal to itself, because f32::NAN is not equal to itself, because (in math) x/0 is not equal to itself (because it is undefined).

That's the most obvious case, but I think there are other cases when dealing with infinity or subnormal numbers. But I don't remember well, I'd have to check.

tqwewe commented 2 years ago

I see, essentially the same reasons why f32 doesn't implement Eq I guess