jcornaz / heron

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

chore: small clippy improvements #173

Closed ManevilleF closed 2 years ago

ManevilleF commented 2 years ago
jcornaz commented 2 years ago

Hi, thanks for the contribution :-) This is very much appreciated.

However, I am sorry, but I will reject this PR.

Here's why:

const functions

I'd rather not eagerly mark everyhting const just because the implementation happens to be a const evaluation. Insted, I would like to wait until someone can share a use-case where a specific const function would be helpful.

Because, once it is marked const, I cannot go back without making a breaking change. So it leaks an implementation detail and restricts my future possibilities.

added clippy rules

All the clippy rules you enabled are already enabled by default: https://github.com/rust-lang/rust-clippy#clippy.

Being explicit about them, only adds clutter to the code IMHO.

[inline]

This is perhaps the most controversial. But, I think in the case of heron specifically, it is just more info in the code, that doesn't bring real value.

First, the end-user can still have the functions inlined (even without any #[inline]) if they enable link-time optimization (lto = true in the cargo profile).

Second, the overall performance cost of heron is anyway relatively expensive. Not only collision/physics are expensive in themselves, but heron has to copy a lot of data back-and-forth between the bevy components and the rapier's world. In-fine, adding more lines of code for such an insignificant optimization sounds a bit odd to me. Overall, if one care about that level of performance, they shouldn't use heron.

mul_add

b.mull_add(c, a) is (IMHO) much harder to read than a + b * c.

I would agree with this change, if there was good testing harness of the code and a benchmark prooving the advantage of the change. But this code is in the debug-renderer, which isn't automatically tested, and that code is anyway meant for debug purposes only, it is not intended to run in production.


So, here it is. Even if I close it, I am thankful for the contribution, and I hope you will understand.