Raph port #20

blu-exe · 2018-11-19T02:28:43Z

No description provided.

new changes merged

BBScholar · 2018-11-19T02:37:31Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/Path.kt

+
+public class Path(filepath: String, backwards: Boolean = false) {
+
+    private var coordinates: MutableList<Vector2> = mutableListOf<Vector2>()


make this mCoordinates

BBScholar · 2018-11-19T02:37:51Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/Path.kt

+public class Path(filepath: String, backwards: Boolean = false) {
+
+    private var coordinates: MutableList<Vector2> = mutableListOf<Vector2>()
+    private var targetVelocities: MutableList<Double> = mutableListOf<Double>()


make this mTargetVelocities

BBScholar · 2018-11-19T02:39:25Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/Path.kt

+    private var coordinates: MutableList<Vector2> = mutableListOf<Vector2>()
+    private var targetVelocities: MutableList<Double> = mutableListOf<Double>()
+
+    private var backwards: Boolean = false


if you're defaulting backwards to false in the constructor, don't set it to false here. Make this line just private var backwards: Boolean and then initialize it in the init block

make it public as well

BBScholar · 2018-11-19T02:45:46Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/Path.kt

+            }
+        } catch (e: FileNotFoundException) {
+            e.printStackTrace()
+            System.exit(1)


Do we exit in the java code? I dont know If we should do that

Since this likely going to be used for autonomous, it makes since to keep running and fall back to a default mode. I think we should let the calling function catch the exception and decide what to do with it.

yeah, it should exit. If the path can't be found, that is a fatal error. Causing the program to exit will force the programmer to resolve the issue before continuing.

no, if it exits in auto, then we cant run in teleop either until the robot reboots.

I agree with Connor

What 254 does is they package all the paths with the jar file. This is probably a better idea, because then the programmer is forced to update the paths locally, as well as catch any errors before the code will work. You will be less likely to run into issues with the paths being out of date or different versions on the robot.

how do you package the paths with the jar?

I know their 2017 code used actual java classes to represent paths

BBScholar · 2018-11-19T02:47:15Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/Path.kt

+        return Vector2.copyVector(coordinates[index])
+    }
+
+    public fun isBackwards(): Boolean {


Use a kotlin getter
val backwards: Boolean get() = field

that get function is on a new line with a tab

val backwards: Boolean get() = field

BBScholar · 2018-11-19T02:48:46Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/Path.kt

+        return tmp
+    }
+
+    public fun getPathLength(): Int {


make this getCoordinatesLength(): Int or something because pathLength could be used for the literally length of the path

BBScholar · 2018-11-19T02:50:43Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/PathFollower.kt

+    }
+    @Suppress("ComplexMethod")
+    private fun calculateLookahead(robotPos: Vector2, robotAngle: Double): Vector2 {
+    mLastClosestPointIndex = mPath.findClosestPointIndex(robotPos, mLastClosestPointIndex)


indentation issue (?). maybe its just github

BBScholar · 2018-11-19T02:53:23Z

src/main/kotlin/frc/team5499/frc2018Kotlin/utils/Vector2.kt

@@ -29,10 +29,51 @@ data class Vector2(val x: Double, val y: Double) {

    fun distanceTo(other: Vector2) = (this - other).magnitude

+    fun getHeading(): Double {


isnt there an angle variable in the vector2 class already? maybe Im wrong. If there isn't, make sure you test this method because it was taken from a different source (Mitchy Boi) so they may not be fully compatible.

there is, figure out how to use that in this. idk if we even need the angle for the vector2 tbh. something to look at

BBScholar · 2018-11-19T02:54:18Z

src/main/kotlin/frc/team5499/frc2018Kotlin/utils/Vector2.kt

+        fun unitDirectionVector(vector: Vector2): Vector2 = vector * 1.0 / vector.magnitude
+
+        @Suppress("MagicNumber")
+        fun copyVector(original: Vector2): Vector2 = Vector2(original.x, original.y)


you don't need this in kotlin if the class is a data class (https://kotlinlang.org/docs/reference/data-classes.html)

BBScholar · 2018-11-19T02:57:30Z

src/main/kotlin/frc/team5499/frc2018Kotlin/utils/Vector2.kt

        fun distanceBetween(a: Vector2, b: Vector2) = (a - b).magnitude
+
+        fun unitDirectionVector(vector: Vector2): Vector2 = vector * 1.0 / vector.magnitude


this is just the normalized vector.There is already a method for this in the vector2 class

see line 10

BBScholar · 2018-11-19T03:00:31Z

src/main/kotlin/frc/team5499/frc2018Kotlin/utils/Vector2.kt

+            Vector2(Math.cos(Math.toRadians(450 - heading)), Math.sin(Math.toRadians(450 - heading)))
+
+        @Suppress("MagicNumber")
+        fun angleBetween(from: Vector2, to: Vector2): Double {


refactor using angle field of each vector

BBScholar · 2018-11-19T03:01:50Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/PathFollower.kt

+        else -mPath.getPointVelocity(mLastClosestPointIndex)
+
+        output[0] = averageVelocity * (2.0 + (curvature * Constants.TRACK_WIDTH)) / 2.0
+        output[1] = averageVelocity * (2.0 - (curvature * Constants.TRACK_WIDTH)) / 2.0


Use a Pair<Double, Double> for this instead of an array. its neater. you'll need to refactor in the auto command as well though

BBScholar

Looks very good. Theres some small issues mostly related to styling. Fix indentation errors for OCS reasons (unless its just github deeking). You added some unnecessary functions to vector2 (probably). And fix the issue with ENCODER_TICKS_PER_ROTATION

andycate

Overall, you've done a pretty good job. Just some organizational and syntax things to fix.

andycate · 2018-11-19T07:00:16Z

src/main/kotlin/frc/team5499/frc2018Kotlin/utils/Vector2.kt

+        }
+        return rawHeading
+    }
+
    override fun toString(): String = "X: $x, Y: $y"

    companion object {
        val Zero = Vector2(0, 0)


make this ZERO

andycate · 2018-11-19T07:02:28Z

src/main/kotlin/frc/team5499/frc2018Kotlin/utils/Vector2.kt

        fun distanceBetween(a: Vector2, b: Vector2) = (a - b).magnitude
+
+        fun unitDirectionVector(vector: Vector2): Vector2 = vector * 1.0 / vector.magnitude


see line 10

codecov · 2018-11-26T06:54:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c601786). Click here to learn what that means.
The diff coverage is 0.86%.

@@           Coverage Diff            @@
##             master     #20   +/-   ##
========================================
  Coverage          ?   1.36%           
  Complexity        ?       6           
========================================
  Files             ?      16           
  Lines             ?     657           
  Branches          ?      39           
========================================
  Hits              ?       9           
  Misses            ?     648           
  Partials          ?       0

Impacted Files	Coverage Δ	Complexity Δ
...in/frc/team5499/frc2018Kotlin/path/PathFollower.kt	`0% <0%> (ø)`	`0 <0> (?)`
...ain/kotlin/frc/team5499/frc2018Kotlin/path/Path.kt	`0% <0%> (ø)`	`0 <0> (?)`
...kotlin/frc/team5499/frc2018Kotlin/utils/Vector2.kt	`31.81% <33.33%> (ø)`	`4 <1> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c601786...d60d2a1. Read the comment docs.

BBScholar

Very close to finished! Just one explicit error and the rest is just convention stuff

BBScholar · 2018-11-30T03:49:56Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/PathFollower.kt

-            var t2: Double = (-b + dis) / (2.0 * a)
+        mLastClosestPointIndex = mPath.findClosestPointIndex(robotPos, mLastClosestPointIndex)
+        var lookahead: Vector2? = null
+        for (i in mLastClosestPointIndex..mPath.getCoordinatesLength()) {


won't this throw an index out of range error since you're using mPath.getPoint(i + 1). Tt should probably be for (i in mLastClosestIndex..mPath.getCoordinatesLength() - 2) (-2 because kotlin using inclusive ranges such as [0, 10] instead of [0, 11) )

A unit test will help with this in the future

BBScholar · 2018-11-30T03:57:31Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/PathFollower.kt


-        return output
+        return Pair(leftOutput, rightOutput)


I would try using a data class instead of a pair to make this less ambiguous to read. https://kotlinlang.org/docs/reference/data-classes.html
for example you could define a public data class within the path follower class like public data class PathFollowerOutput(val leftVelocity: Double, val rightVelocity: Double) (thats all you need) and then you could simply return it as PathFollowerOutput(leftOutput, rightOutput). Then, retrieving the values wont be as confusing later. It would be output.leftOutput instead of output.first. Personal opinion though.

BBScholar · 2018-11-30T03:59:51Z

src/main/kotlin/frc/team5499/frc2018Kotlin/path/Path.kt


-    private var backwards: Boolean = false
+    private var backwards: Boolean


use val please

for mCoordinates and mTargetVelocities as well

blu-exe added 11 commits November 3, 2018 18:03

create Path.kt

275c11c

test

8139fba

back to original!

8a8fde3

up to line 98 of origin code

e1f6bd1

Merge remote-tracking branch 'origin/master' into raphPort

b07f02b

new changes merged

basic porting

5ea655d

restart!

461b7fa

merge with master

aaf3d4c

Merge remote-tracking branch 'origin/master' into raphPort

b11dd15

fixed some style errors

9366f3e

fixed MaxLineLength and MagicNumber

b985142

blu-exe self-assigned this Nov 19, 2018

blu-exe requested review from connorworley, andycate, BBScholar and nicholasunderwood November 19, 2018 02:28

BBScholar reviewed Nov 19, 2018

View reviewed changes

BBScholar requested changes Nov 19, 2018

View reviewed changes

andycate requested changes Nov 19, 2018

View reviewed changes

blu-exe added 2 commits November 25, 2018 21:10

fixed formatting, redundancies

4aa4d2e

minor fix

cab4114

blu-exe added 2 commits November 27, 2018 21:05

SuppressWarnings, minor fixes

b2dfa3d

whoops very minor change

fe368f5

BBScholar requested changes Nov 30, 2018

View reviewed changes

fixed conventions, output

d60d2a1


		public class Path(filepath: String, backwards: Boolean = false) {

		private var coordinates: MutableList<Vector2> = mutableListOf<Vector2>()

		@@ -29,10 +29,51 @@ data class Vector2(val x: Double, val y: Double) {

		fun distanceTo(other: Vector2) = (this - other).magnitude

		fun getHeading(): Double {

		fun distanceBetween(a: Vector2, b: Vector2) = (a - b).magnitude

		fun unitDirectionVector(vector: Vector2): Vector2 = vector * 1.0 / vector.magnitude


		private var backwards: Boolean = false
		private var backwards: Boolean

Raph port #20

Are you sure you want to change the base?

Raph port #20

Conversation

blu-exe commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BBScholar Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andycate Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BBScholar left a comment • edited Loading

Choose a reason for hiding this comment

andycate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 26, 2018 • edited Loading

Codecov Report

BBScholar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BBScholar Nov 30, 2018 • edited Loading

Choose a reason for hiding this comment

BBScholar Nov 19, 2018 •

edited

Loading

andycate Nov 19, 2018 •

edited

Loading

BBScholar left a comment •

edited

Loading

codecov bot commented Nov 26, 2018 •

edited

Loading

BBScholar Nov 30, 2018 •

edited

Loading