Fixed bug in server.rules.math::calcNewBotSpeed

[mnbrandl]

Previously, when the currentSpeed = 0, it was possible to accelerate backwards at a limit bound by DECELERATION, rather than ACCELERATION.

PREVIOUS OUTPUT:
    ...
    calcNewBotSpeed(0, -0.9) -> -0.9
    calcNewBotSpeed(0, -1)   -> -1
    calcNewBotSpeed(0, -1.1) -> -1.1
    ...
    calcNewBotSpeed(0, -1.9) -> -1.9
    calcNewBotSpeed(0, -2)   -> -2
    calcNewBotSpeed(0, -2.1) -> -2
    ...

The condition when currentSpeed = 0 is now evaluated separately from the condition when currentSpeed > 0.

CURRENT OUTPUT:
    ...
    calcNewBotSpeed(0, -0.9) -> -0.9
    calcNewBotSpeed(0, -1)   -> -1
    calcNewBotSpeed(0, -1.1) -> -1
    ...
    calcNewBotSpeed(0, -1.9) -> -1
    calcNewBotSpeed(0, -2)   -> -1
    calcNewBotSpeed(0, -2.1) -> -1
    ...

[flemming-n-larsen]

@mnbrandl Thank you for the contribution and help fixing an issue. 😊👍

Before merging this into the main branch, I will first create some tests that identifies the bug, but also suit for refactoring code later etc. So please be patient with me till I have some test ready.

[mnbrandl]

@flemming-n-larsen

Glad to help!

I suppose I should add, I was getting an error that Gradle was unable to locate the kotlin-stdlib dependency, as no repositories had been added. I'm assuming it is probably an issue to my specific build environment (I'm having other issues trying to build), but maybe not. Whatever the issue, it was fixed by adding a repositories block to the dependencyResolutionManagement block in settings.gradle.kts. Not being very Gradle saavy, I don't know if this is something that needs to be looked into.

As for the tests, I have written some code that involves using the method, but did so in my own project for developing bots. The code wasn't written as a test, but it happened to function as a test find and fix the bug.
The identifying feature of the bug was that the output was not symmetrical for inputs that were negated. I.e. the magnitude of the output for inputs (0, 8)-> (1) was not the same magnitude of the output for inputs (0, -8)->(-2).

I am willing to come up with some tests, but I'm not sure how to proceed given your comment about being suited for future refactoring.

[flemming-n-larsen]

@mnbrandl

I am willing to come up with some tests, but I'm not sure how to proceed given your comment about being suited for future refactoring.

I am adding some tests very soon (work in progress) that proves that there is a bug, meaning that some of the tests will fail.
After that, I will merge your fix to main, and the test should all get green. 🙂

[flemming-n-larsen]

@mnbrandl I have now added some basic tests that proves that there are several issues:
1258a6a

The issue you found with the speed being -2 instead of -1.

But also some issue when the speed is going from a positive to negative value, and vice versa, and comparing with the behaviour of the original Robocode.
The original Robocode use the getNewVelocity(), so we might lack the decelTime calculation or something like that. So I will have a look at this as well. 😊

[mnbrandl]

@flemming-n-larsen

I did not think to compare behavior with the original Robocode, that would have made too much sense! I do believe you are correct, the decel time needs to be calculated (from which we can easily obtain the accel time as well). The new speed is simply acceleration * time. However, to correctly calculate the new position, the time accel and decel time is also required in the method MutableBot::moveToNewPosition.

Also, in instances crossing zero from a given current speed where the target speed could result in time spent decelerating, accelerating, and traveling at constant speed (eg. -0.5, 0.5), which would also be required in moveToNewPosition.

I see the original is doing things differently, relying on the distance remaining rather than a target speed to calculate movement. Target speed is now calculated in the BaseBotInternals class, and seems to be doing much of the work we are looking to implement in the server module. I'll have to look more into myself as well, let me know your thoughts once you have taken a look!

[flemming-n-larsen]

@mnbrandl I merge you pull request now as promised. Then I will figure out how to incorporate the crossing of zero without the distance so we can fix the remaining part of the test as it would be best to let tank royale be as compatible with the original game as possible to support the bridge. Especially because they are so similar.

[flemming-n-larsen]

@mnbrandl I fixed the remaining bug when speed 0 is crossed, i.e. when the current speed and target speed have different signs. 😊