Skip to content

Conversation

@PickleFace5
Copy link
Contributor

@PickleFace5 PickleFace5 commented Nov 18, 2024

On robot initialization, the console output would have the following warning occasionally:

Error at `anonymous-namespace'::WPILibMathShared::ReportErrorV: Error: x and y components of Rotation2d are zero

After some print debugging, I found this was caused by forceVec being an empty Translation2d. This simply adds a check for the forceVec Translation2d to avoid sending warnings to the console.

This only updates PPLib Python, I'm unaware if this issue is present in other languages.

@github-actions github-actions bot added the PathPlannerLib Changes to PathPlannerLib label Nov 18, 2024
@mjansen4857
Copy link
Owner

This needs to be added to Java and C++ as well. There is also an identical location that needs this check in the reverse accel pass method. Also, can you check for a 0,0 translation by checking if the norm is <= 1E-6 instead of equality? This will match the implementation of this check for other locations in the library.

@PickleFace5
Copy link
Contributor Author

PickleFace5 commented Nov 22, 2024

I have added checks to Java and C++ with the recommended implementation. It has also been added to the reverse accel pass as well.

@PickleFace5 PickleFace5 changed the title Add Empty Translation check for forceVec Add Empty Translation Checks Nov 22, 2024
@mjansen4857
Copy link
Owner

Looks like build is failing just for some unit stuff. You just need to add an extra () after the C++ Norm() calls. So translation.Norm()() <= 1E-6

To fix formatting, you can do ./gradlew spotlessApply

@mjansen4857 mjansen4857 added this to the 2025 Beta 5 milestone Nov 23, 2024
@mjansen4857
Copy link
Owner

Also, can you replace usages of new Rotation2d() with Rotation2d.kZero in Java? Helps reduce memory pressure a bit.

@PickleFace5
Copy link
Contributor Author

@mjansen4857 added your requested changes. Hopefully that was the only syntax error for C++, I'm having issues with VS Code finding the header files. 😅

@mjansen4857 mjansen4857 merged commit 61cde90 into mjansen4857:main Nov 25, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PathPlannerLib Changes to PathPlannerLib

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants