search

acmerobotics / road-runner

Wheeled mobile robot motion planning library designed for FTC
MIT License
209 stars 75 forks source link

Implemented vector cross product #71

Closed optimisticside closed 2 years ago

optimisticside commented 3 years ago

I've implemented functionality to find the cross product between 2 Vector2D's.

rbrott commented 3 years ago

Thanks for your contribution. However, I don't want to add vector methods that aren't used by the rest of the module. I'd be more open to a change like this if you could identify existing "inlined" cross products and replace them for clarity.

As an aside, there's something wrong with the implementation: cross() is not anti-commutative.

henopied commented 3 years ago

I would recommend finding candidates for this with a project search with regex turned on using this regex \w+\.(?:x|y) \* \w+\.(?:x|y) This search also finds some places where there are inlined dot products.

optimisticside commented 3 years ago

I ran the regexp and found 19 matches. Should I change them to use the cross function?

rbrott commented 3 years ago

I ran the regexp and found 19 matches. Should I change them to use the cross function?

No, there are probably a few false positives. But more importantly, I'm only open to adding cross() if it materially improves the readability of the source. I'm not really interested in adding it purely for the convenience of users in their code.

optimisticside commented 3 years ago

I believe that adding this would improve the readability of the source code. Just like doing a dot b, doing a cross b allows for more readability as opposed to what may, at times, seem somewhat obscure.

val a = v0.x * v1.y - v0.y * v1.x
val b = v0 cross v1
rbrott commented 2 years ago

I regret my abrasive responses earlier in this thread. They were unmerited and antithetical to the welcoming contribution environment I hope to cultivate here. If you're still interested in this patch, I'm happy to comb through the instances flagged by the regex search.