jcornaz / heron

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

Do not add TimePlugin #320

Closed elsid closed 2 years ago

elsid commented 2 years ago

If application adds TimePlugin too, this breaks time delta computation. See this comment for more details.

Applications should add all required plugins before adding heron plugins.

elsid commented 2 years ago

I don't agree to make the change without breaking the API. Let me explain.

I've found the issue when tried to update my project from bevy 0.7 and heron 3.1.0 to 0.8.1 and 4.0.2 respectively. For me 4.0.2 did a breaking change because of adding TimePlugin by RapierPlugin (TimePlugin didn't exist before bevy 0.8). I'm not sure whether it was it intentional but I think I'm not the only one who faced this problem. Bevy documentation suggests to use its plugin groups like MinimalPlugins and DefaultPlugins depending on the use case and both add TimePlugin. I expect most of applications to follow this approach. Maybe there are applications which cherry-pick specific plugins and don't use plugin groups. IIUC your concern is about these applications which will break by this pull request because they don't add TimePlugin and rely on RapierPlugin doing it. I'm not sure it's a good idea to avoid breaking API change just to keep current potentially incorrect behaviour for the benefit of such projects. This only complicates the update from 3.1.0 to 4 for regular users.

Fundamental problem here is probably a lack of mechanism to prevent plugin duplication in bevy or basically there is no plugin dependency management (https://github.com/bevyengine/bevy/issues/69). I think https://github.com/jcornaz/heron/pull/301 did a mistake without understanding the problem in attempt to fix tests. Unfortunatelly there is no cheap fix for such mistakes. Breaking API change could save time for developers who are not aware of the problem yet which I think is better than providing smooth update for applications already using 4.0.2.

jcornaz commented 2 years ago

Yes, it was a mistake to install the TimePlugin. But why this mistake was made though? It is my conviction, that the root cause is how bevy carelessly breaks its API all the time and put pressure on plugin authors who have to upgrade ASAP because their plugin suddenly became obsolete.

I am aware of the technicalities of plugin management with bevy. But that's an unimportant implementation detail.

I feel very strongly against breaking changes in general, and I am very unhappy with how bevy treats them. In fact, that's why I decided to discontinue heron.

I do not want to behave as they do. Adding a new plugin, and deprecating the old one is very graceful. All users get notified that the old one is deprecated and can easily use the new one, without being forced to take immediate action if they don't want to.

I am sorry, but I won't merge a breaking change. There will not be a heron 5.0.0.

jcornaz commented 2 years ago

Just a quick addition to my previous message, as I'd like to respond to this point in particular:

This only complicates the update from 3.1.0 to 4 for regular users.

Publishing heron 5 would not make it easier for migrating from 3 to 4. But a non-breaking change (in 4.1) would help, as the users would see the deprecation and could understand quicker what is the problem.

And the very reason why users even have to migrate is because of the breaking changes in bevy 0.8 and heron 4. If it wasn't for those breaking changes the problem would not exist in the first place. Which is just yet another fact supporting my point: we should not make breaking changes!

Finally, let me say that we have no idea how many people may be broken. It may be nobody, but it may be close to everybody (users may have counterbalanced the problem in one way or another, maybe without even realizing it).

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 5.0.0 :tada: