Hi,
Il 28/05/21 10:37, Nikolay Sivov ha scritto:
These two changes (or three) changes are separate. Second changes default even when none of components support rate change. How did you test this?
I don't understand why they should be separate. This piece of code is basically computing the minimum or the maximum between a bunch of values.
There were two bugs: first one is that 0.0 is a valid initialization value when you're computing a maximum, but not when you are computing a minimum (if you iteratively compute min() starting from 0.0, the result will always be 0.0).
Second bug is that you have compute the maximum and minimum in absolute value. For example, if a session has a source with fastest reverse rate -100.0 and another with -20.0, the global fastest should be the minimum in absolute value, which is -20.0, and not -100.0.
My patch fixes two bugs, but they are in the same algorithm, so it felt right to have them together.
As for the tests, again, it's never clear to me when it's appropriate to have tests and when not. I would be inclined to always add tests, the more the better, but given other reviews I received, this is not necessarily true. I can add tests, but they will require a few mock classes in order not to be trivial. Let me know if this is welcome or not.
Lacking tests, I fixed this algorithm based on what it could logically do. Since the point is finding the set of rates that are acceptable for all the components in the session, and that reverse rates are represented with negative numbers, it felt right to implement the algorithm that I described above (compute the maximum absolute value when dealing with slowest rates and the minimum absolute value when dealing with fastest rates). Also, this new algorithm makes Deep Rock Galactic working, which it didn't before.
Giovanni.