feat: allow to define layers for collision shapes generated by `PendingConvexCollision`

[Shatur]

This PR adds an additional field to the PendingConvexCollision to set collision layers. I realized that setting default collision is needed only for the generated collision. In all other cases user can define needed collision layer explicitly. Closes #217.

[jcornaz]

Hi, thanks for the contribution.

What is the advantage to have it as a field of the PendingConvexCollision instead of inserting the component directly?

[Shatur]

What is the advantage to have it as a field of the PendingConvexCollision instead of inserting the component directly?

PendingConvexCollision creates collision recursively for scene meshes. If I just insert CollisionLayers to the entity with PendingConvexCollision - it won't work.

[Shatur]

@jcornaz So this parameter works like current body_type parameter. The specified layers will be inserted into every mesh entity in spawned scene.

[jcornaz]

Maybe we could define that if a CollisionLayers component is present in the entity with PendingConvexCollision that is the layer to apply in all collision shapes to spawn?

[Shatur]

@jcornaz We can, but I don't think this behavior is very obvious. It will also be inconsistent with the body_type parameter. I would introduce a small breakage for the benefit of a more nicer API.

[jcornaz]

I want to try as hard as I can to avoid breaking changes, no matter how "small" it is. (EDIT: And it is not yet clear what would be the nicest API anyway)

But I agree with your point about how obvious it would be. I only partially agree with your point about consistency, yes it would be inconsistent, but we may fix that differently by changing how the body type is defined instead (and deprecating the rigid_body field in PendingCollisions)

Maybe we could introduce a new type that would explicitly hold the configuration for that feature. Maybe something like:

struct CollisionGenerationConfig {
  body_type: RigidBody,
  layers: CollisionLayers,
}

And one could configure the behavior of the PendingConvexCollision with that struct.

Obviously, we would make sure to keep the fields private (or mark the struct #[non_exhaustive]) so that we can extend the configuration in the future.

[jcornaz]

Or, we can add a new component that replaces the existing PendingConvexCollision and deprecate the old one.

Obviously, we should try to not redo our mistake to have the field public, so that we can extend it later.

[Shatur]

Maybe we could introduce a new type that would explicitly hold the configuration for that feature. Maybe something like:

Could you explain this a bit more? Not sure if I get it.

Or, we can add a new component that replaces the existing PendingConvexCollision and deprecate the old one.

I understand your point about breakages, it's a good approach. We should always think about how to do it without breakages. Sometimes it even leads us to better ideas. But in this particular case I don't see a lot of difference between deprecated struct and a newly added field. On my CI warnings are marked as errors. It's a trivial change, I can understand what needs to be changed from the compilation error, I don't need a deprecation message. Bevy itself (for which this plugin is written) is also making breakages. So I would expect breaking changes at early development stage.

[jcornaz]

Maybe we could introduce a new type that would explicitly hold the configuration for that feature. Maybe something like:

Could you explain this a bit more? Not sure if I get it.

We could create the following component:

#[derive(Default, Component)]
struct CollisionGenerationConfig {
  body_type: RigidBody,
  layers: CollisionLayers,
}

impl CollisionGenerationConfig {
  fn new(body_type: RigidBody) -> Self { ... }
  fn with_layers(self, layers: CollisionLayers) -> Self { ... }
}

When inserting a PendingConvexCollision one could also insert a CollisionGenerationConfig component in the same entity. the collision shape generation system would query. Query<(&PendingConvexCollision, Option<&CollisionGenerationConfig>)> and respect the given config to create the shapes.

But in this particular case I don't see a lot of difference between deprecated struct and a newly added field. On my CI warnings are marked as errors.

Deprecating is not a breaking change. Adding a new field to that struct is a breaking change. That is a massive difference to my eyes. Because it is deprecated doesn't mean you cannot use it. If one is using it, they can migrate it their leisure. But I don't enforce anything on the consumers. Of course, some users may have their CI fails if they treat warnings as an error (which is a good idea, and I do that too) but the big difference is: it is their call. They decide if they want to treat it as a failure or not, and they can even decide to silence that specific warning without migrating if they want to.

It's a trivial change, I can understand what needs to be changed from the compilation error, I don't need a deprecation message.

I think it is no less trivial to change with the deprecation. I would argue it is even more trivial since you don't have to change it at all if you don't want to. If if is a breaking change, you at least need to spend the cognitive effort to understand what is the new API. That can never be more trivial than doing nothing, or adding #[allow(deprecated)].

Bevy itself (for which this plugin is written) is also making breakages.

I love bevy. But that's not an area where bevy is my model. At all. I will not follow the bevy way of treating breaking changes.

Of course, for each breaking release of bevy, heron will also make a breaking release. Because the complexity that would be needed to make less breaking changes than bevy is not worth the benefits. But ideally, heron would only break its API for major bevy updates, and nothing else. That would be my aim. Of course I am here talking about an ideal to aim for. We may in practice break the API more often. But I just want to avoid them as much as I possibly can. And for this feature, I feel like it is possible, even easy to avoid breakage.

[Shatur]

When inserting a PendingConvexCollision one could also insert a CollisionGenerationConfig component in the same entity.

Get it now, thanks. But why CollisionGenerationConfig contains RigidBody? We have it in PendingConvexCollision. But as to end user, this behavior makes the API less clear...

Deprecating the PendingConvexCollision and using a new one with builder patter looks also less consistent to me. Because in Bevy bundles usually plain structures.

That is a massive difference to my eyes. Because it is deprecated doesn't mean you cannot use it. If one is using it, they can migrate it their leisure. If if is a breaking change, you at least need to spend the cognitive effort to understand what is the new API

That's true for massive changes. Not migrating to a new API can be useful when it is totally different and the transition results in a change in the application structure. In this case delaying transition worth it. But how it could be useful with minor change? I would migrate to a new API if that easy. Yes, it's better to avoid it, but other solutions looks less nicer as to me. So I would sacrifice compatibility for API sanity.

But it just my opinion. I will do as you say if you agree with the feature in general :)

[jcornaz]

But why CollisionGenerationConfig contains RigidBody? We have it in PendingConvexCollision. But as to end user, this behavior makes the API less clear...

We could deprecate the rigid_body in PendingConvexCollision. The advantage is consistency. Basically I made that proposition to address that point you made:

It will also be inconsistent with the body_type parameter

Which I agree with, hence my attempt to suggest a solution that addresses that concern.

Deprecating the PendingConvexCollision and using a new one with builder patter looks also less consistent to me. Because in Bevy bundles usually plain structures.

Yes, I agree. But as I mentioned in my previous message, I disagree with how bevy treats breakages, and I don't want to replicate what I consider to be a mistake in bevy. Using plain structures with all public fields leads to the necessity of breaking changes, which I don't want.

In other words: Yes it is inconsistent with bevy. But I think that avoiding breaking changes is more important than being consistent with bevy.

That is a massive difference to my eyes. Because it is deprecated doesn't mean you cannot use it. If one is using it, they can migrate it their leisure. If if is a breaking change, you at least need to spend the cognitive effort to understand what is the new API

That's true for massive changes. Not migrating to a new API can be useful when it is totally different and the transition results in a change in the application structure. In this case delaying transition worth it. But how it could be useful with minor change?

I agree that it is even more important for massive changes. But I don't think it becomes "false" for small changes. And small/big is kinda subjective. Because we don't know from where they update. If a user jumps from version 1.0.0 to 10.0.0 they may be a lot of "small" breaking changes, which together makes it big.

I would migrate to a new API if that easy.

Yes, me too. But I believe it should be up to the user to make that decision. I don't want to impose it on them.

So I would sacrifice compatibility for API sanity.

Me too... kinda. Let's say that I may agree to make a breaking change if absolutely required for the quality of the API.

But I don't think it is the case here. Potentially we can make the API even better than it would be by adding a field in PendingConvexCollisions.

I don't see deprecation as hurting the API quality. I consider that as a graceful removal from the official API. Existing users are well informed that it is no longer supported and new users don't have to see it at all (we can and should hide it from the docs).

For this PR, I suggest the following course of action:

• Create a new CollisionShapeGeneration component, that is meant to be the successor of PendingConvexCollision
  • The content is a replication of the existing PendingConvexCollision (plus layers)
  • with private fields
  • builder-like way of setting the fields.
• Deprecate and hide from the docs PendingConvexCollision

For the system implementation, we can consider many approaches. I would suggest implementing From<PendingConvexCollision> for CollisionShapeGeneration.

• For each entity with CollisionShapeGeneration: Execute the collision shape generation normally.
• For each entity with PendingConvexCollision and without CollisionShapeGeneration:
  • Convert the PendingConvexCollision into a CollisionShapeGeneration and use it to execute the collision shape generation normally.

We may also do that in multiple PRs and even share the load. For example, I could introduce CollisionShapeGeneration and deprecate of PendingConvexCollision without any change of feature. And then you can add support for collision layers.

I am still very open for the design of the API and it's implementation. Only when it comes to breaking changes, I am quite settled, that this feature doesn't need nor deserve an API breakage.

[Shatur]

Hmm... You convinced me :) This actually a good approach, I like your vision.

You implementation strategy is also sounds good to me. I will happy to implement it. But I just discovered another solution that probably worth to consider. What if we deprecate only rigid_body field in PendingConvexCollision and switch to a component-based approach? You suggested similar thing in this post, but with rigid_body deprecation it starting makes sense to me. So instead of using builder API to avoid API breakages in the future, users will insert the necessary components, to an entity with PendingConvexCollision which will be transferred to child meshes and removed from the scene root. This approach is just more ECS-like and will require to deprecate only one field with minimum changes in code to support both approaches. What do you think?

[jcornaz]

Yes, that's close to the idea I had indeed, and that sounds good to me. It is probably the best solution. I think we can go with that.

[Shatur]

Great! I will update the PR today after work.

[Shatur]

Done! I also added Default implementation for easier creation. Should I provide new method as well?

[jcornaz]

Should I provide new method as well?

No, default is enough.

[jcornaz]

You can #[allow(clippy::type-complexity)]

[Shatur]

Thanks!