wpilibsuite / PathWeaver

Desktop application for generating motion paths
Other
64 stars 68 forks source link

Left and Right paths are swapped #137

Closed icemannie closed 5 years ago

icemannie commented 5 years ago

The attached files are from a path drawn from the left load station to the left of the cargo ship. See the images. The left and right csv files (couldn't attach) show the right traveling 32 ft and the left 28 ft although the left is on the outside of the path. Also the heading is positive although Pathfinder convention has it negative.

This and other paths don't run correctly on our test robot but equivalent Vannaka Motion Profile Generator generated paths do. left path right path

angle2 point1 point2

Windows laptop, V10, Java 11 Path Weaver 2019.2.1

Additional context
Add any other context about the problem here.

SamCarlberg commented 5 years ago

This is most likely just a problem with the pathfinder library. Can you try adding an intermediate waypoint to better define the path?

icemannie commented 5 years ago

I added another point so the path didn't run through the rocket with the same results. The right path was longer than the left and the heading was increasing positive. I tried another path with a clockwise turn where right should have been more than left and it isn't. The heading went negative when I would have expected plus. I know from last year that we had to negate the navX yaw reading because a clockwise turn with it was a positive value while Pathfinder standard was clockwise negative. We were running Vanaka's Motion Profile Generator last year but switched to Path Weaver this year. We couldn't get the simplest turns to run until we switched back to Motion Profile Generator.

On Mon, Jan 21, 2019 at 1:54 PM Sam Carlberg notifications@github.com wrote:

This is most likely just a problem with the pathfinder library. Can you try adding an intermediate waypoint to better define the path?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wpilibsuite/PathWeaver/issues/137#issuecomment-456183511, or mute the thread https://github.com/notifications/unsubscribe-auth/ASWROtGWu0ugYb4Ty80ZBMC3tyRunCp8ks5vFhrogaJpZM4aLajM .

kjrobrien commented 5 years ago

Would you mind uploading your project or sharing the values of the points you are using so we can recreate this on our side? Thanks!

icemannie commented 5 years ago

TempPW.zip Zip folder of modified original path and second added test path

icemannie commented 5 years ago

Done. Let me know if more is needed.

On Mon, Jan 21, 2019 at 4:13 PM kjrobrien notifications@github.com wrote:

Would you mind uploading your project or sharing the values of the points you are using so we can recreate this on our side? Thanks!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wpilibsuite/PathWeaver/issues/137#issuecomment-456212121, or mute the thread https://github.com/notifications/unsubscribe-auth/ASWROhkzRHQ9PWAe4It4i5VriFPiMVwLks5vFjuWgaJpZM4aLajM .

gftabor commented 5 years ago

Okay I am looking at the CSVs themselves (don't have pathweaver on my work computer) and it seems the sides might be flipped (left vs right).

Looking at the Angle2 we see left Y: 3.297195 -> 10.659278 right Y: 1.127195 -> 10.689001

left X: 0.728068 -> 20.798096 right X: 0.728195 -> 22.967892

Logically the 2 sides should start at the same X and end at the same Y, which we see. However the left starts closer to the corner (0,0) than the right side and that is not the case in the CSVs.

image

Exactly whose code or where might be causing this is unclear. @icemannie does this seem consistent with your findings?

icemannie commented 5 years ago

Great catch! Hadn’t noticed that but you are right. That brings up another question though. Why is the y origin at the top left of the Path Weaver image? Earlier programs have it bottom left. If the intention was for it to be at the bottom, right would be below left.

One thing I like to do is an Excel XY scatter diagram to double check the path. With the y origin at the top, it flips the curves.

My biggest question until today was the inverted heading from Pathfinder question.

I should say at this time, I really like some of the features of Pathweaver like path groups and the dragging the path.

John

On Jan 21, 2019, at 4:52 PM, Griffin Tabor notifications@github.com wrote:

Okay I am looking at the CSVs themselves (don't have pathweaver on my work computer) and it seems the sides might be flipped (left vs right).

Looking at the Angle2 we see left Y: 3.297195 -> 10.659278 right Y: 1.127195 -> 10.689001

left X: 0.728068 -> 20.798096 right X: 0.728195 -> 22.967892

Logically the 2 sides should start at the same X and end at the same Y, which we see. However the left starts closer to the corner (0,0) than the right side and that is not the case in the CSVs.

Exactly whose code or where might be causing this is unclear. @icemannie does this seem consistent with your findings?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

gftabor commented 5 years ago

So in answer to your "why is it this way" question. The way we handle user units is (quite clever in my opinion) we make a pane that is the size of the field in your desired units, but drawn on top of the field image. So JavaFX is handling the conversion between drawn pixels, picture pixels and user feet for instance when drawing and devs don't have to think about it.

This means that if I ever made a waypoint print its location visually in PathWeaver it would all be in user desired units, that is the "native" units of everything you see drawn in path weaver other than the image. It saves a lot of bugs related converting back and forth between visual units and user units.

It does have the consequence of (0,0) being in the upper left corner without constantly adjusting for it. Which honestly hadn't been a negative until this moment.

I guess I never noticed because I have worked with images before and just kinda assumed that's where (0,0) was. positive y being down (as is standard in images) vs positive y being up (as is standard everywhere else)

In lastly in terms of your point that this mixup between standards of positive vs negative y is the cause of left vs right being flipped. Ya your probably right. Needs to be investigated to see if flipping left/right back is always the correct answer(as I assume) or if changing our coordinate frame is necessary to permanently fix this issue.

icemannie commented 5 years ago

https://www.chiefdelphi.com/t/pathfinder-coordinate-system/159870/7

John

On Jan 21, 2019, at 5:51 PM, Griffin Tabor notifications@github.com wrote:

So in answer to your "why is it this way" question. The way we handle user units is (quite clever in my opinion) we make an pane that is the size of the field in your desired units, but drawn on top of the field image. So JavaFX is handling the conversion between drawn pixels, picture pixels and user feet for instance when drawing and devs don't have to think about it.

This means that if I ever made a waypoint print its location visually in PathWeaver it would all be in user desired units, that is the "native" units of everything you see drawn in path weaver other than the image. It saves a lot of bugs related converting back and forth between visual units and user units.

It does have the consequence of (0,0) being in the upper left corner without constantly adjusting for it. Which honestly hadn't been a negative until this moment.

I guess I never noticed because I have worked with images before and just kinda assumed that's where (0,0) was. positive y being down (as is standard in images) vs positive y being up (as is standard everywhere else)

In lastly in terms of your point that this mixup between standards of positive vs negative y is the cause of left vs right being flipped. Ya your probably right. Needs to be investigated to see if flipping left/right back is always the correct answer(as I assume) or if changing our coordinate frame is necessary.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

gftabor commented 5 years ago

https://www.chiefdelphi.com/t/pathfinder-coordinate-system/159870/7 John

Yup, the question is how to go about fixing the issue. We want to avoid breaking every path that anyone has made using pathWeaver. So minimum impact fixes are preferred. I believe just flipping left and right paths when we save them would be the only required change. (but needs someone to actually check) More drastic changes would change all the related path files and anyone with drawn paths now would have their paths magically move when they updated.

icemannie commented 5 years ago

First thing is to get it working correctly. I would question whether swapping left and right is enough. The heading is incorrect also. I have attached csv's from an almost identical Moton Profile Generator Path showing a - heading. Heading is used in correcting the path of the robot so it needs to be right or the robot won't follow correctly. The only Chief Delphi post I have noticed on Path Weaver is from a team just learning trajectories. If this isn't a real sshort term fix it would be better to let teams know. Personally I would rather redo my trajectories and have the coordinate system standardised. We anticipate maybe 5 or 6 different one this year. Anyone doing more likely does their own path planning. If you need any testing done, I would be happy to do that.

LLDToCS_left.zip

kjrobrien commented 5 years ago

Would you mind testing the following: When following a path, swap the left and right values and multiply the heading by -1?

icemannie commented 5 years ago

That certainly makes the Excel charts of the csv's look right and will likely fix things but need to patch my code and test to confirm. I will try to get robot access this morning. Not sure we will be meeting tonight because of weather.

On Mon, Jan 21, 2019 at 11:29 PM kjrobrien notifications@github.com wrote:

Would you mind testing the following: When following a path, swap the left and right values and multiply the heading by -1?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wpilibsuite/PathWeaver/issues/137#issuecomment-456275955, or mute the thread https://github.com/notifications/unsubscribe-auth/ASWROi9W7hbHz715SGcRQH-r8vWYS44Uks5vFqHUgaJpZM4aLajM .

icemannie commented 5 years ago

I was able to test simple left and right turns on the robot with the left side following the right path and vice versa and with heading negated. Everything worked correctly. I would be happy to test out the fix once you have it available.

kjrobrien commented 5 years ago

John, Per this post: https://www.chiefdelphi.com/t/pathfinder-fast-motion-profiling-for-java-and-c-tank-and-swerve-drive/150942/23?u=kjrobrien I believe the heading should not be negated given our orientation of the coordinate system.

Positive headings are going from X+ towards Y+

This is what is happening in the path Angle2 that you uploaded, so I do believe the headings are correct given our orientation of the coordinate system. (Whether that system makes sense going forward is up for debate :) ) Now, this may require users to orient themselves to our axis in their follower code (by negating the angle, etc.).

However, the left and right paths are being swapped, unfortunately. I have attached a .zip file for a build of PathWeaver that I'd like you to try and see if the output paths are in line with what you would expect. If you have the time, we would love if you could try loading the outputs onto a robot. If this behaves as expected, my goal would be to release this in the next update, with a note informing users that paths should be regenerated. pathweaver-2019.2.1-invert-fix.zip

. Thanks for your help & guidance, Kevin

icemannie commented 5 years ago

The college where we have our robot is closed today for weather conditions. It may be open this evening. I will build and look at the CSVs later this morning. One change that you should - even must - make to help the change get accepted is to add a grid ruler to x and y axes on the image. I simply made the assumption that y0 was bottom left and struggled with the paths not performing on the robot.

icemannie commented 5 years ago

Kevin,

Teams using Path Weaver need a fix now. A six week build season is short enough. I have posted on Chief Delphi accordingly. Instead of rushing a release, why not get out a Team Update that explains there is a problem, and how to get around it. If teams would still have to make one change in code to invert heading, it would be straightforward enough to have them either also read the left csv into the right Trajectory and vice versa or read the left and right Trajectories into the right and left Distance Followers.

Path Weaver can still be used for plotting the paths.

On Tue, Jan 22, 2019 at 11:22 PM kjrobrien notifications@github.com wrote:

John, Per this post: https://www.chiefdelphi.com/t/pathfinder-fast-motion-profiling-for-java-and-c-tank-and-swerve-drive/150942/23?u=kjrobrien I believe the heading should not be negated given our orientation of the coordinate system.

Positive headings are going from X+ towards Y+

This is what is happening in the path Angle2 that you uploaded, so I do believe the headings are correct given our orientation of the coordinate system. (Whether that system makes sense going forward is up for debate :) ) Now, this may require users to orient themselves to our axis in their follower code (by negating the angle, etc.).

However, the left and right paths are being swapped, unfortunately. I have attached a .zip file for a build of PathWeaver that I'd like you to try and see if the output paths are in line with what you would expect. If you have the time, we would love if you could try loading the outputs onto a robot. If this behaves as expected, my goal would be to release this in the next update, with a note informing users that paths should be regenerated. pathweaver-2019.2.1-invert-fix.zip https://github.com/wpilibsuite/PathWeaver/files/2785772/pathweaver-2019.2.1-invert-fix.zip

. Thanks for your help & guidance, Kevin

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wpilibsuite/PathWeaver/issues/137#issuecomment-456674248, or mute the thread https://github.com/notifications/unsubscribe-auth/ASWROmrYZxCJRSulQJX9aAIY6_COCkifks5vF_F5gaJpZM4aLajM .

bradamiller commented 5 years ago

John - thanks for all your help testing and tracking down the problem. Seems like the right thing to do is let people know that the issue exists today so they can make the 2 line change to their code to work around the problem. Then we can fix it correctly in the next update with a clear description of the issue in the release notes.

icemannie commented 5 years ago

Brad, I agree wih that and will not test the version Kevin sent me. I posted on Chief Delphi this morning about the situation and so far at least not a single comment. That and no earlier questions seems to indicate low usage currently or teams aren't at that point yet. Perhaps the driver control in sandstorm autonomous is making teams less inclined to make the effort. I have tested Vannaka Motion Profile Generator 4.0 and it works with standard equations.

kjrobrien commented 5 years ago

This decision has been reflected in the documentation on screensteps here: https://wpilib.screenstepslive.com/s/currentCS/m/84338/l/1021631-integrating-path-following-into-a-robot-program and has been documented on the list of known issues here: https://wpilib.screenstepslive.com/s/currentCS/m/getting_started/l/1028964-known-issues

wrall commented 5 years ago

I'm not sure that the solution of swapping left and right paths is universally the case. I have some paths that work correctly and some that don't. My paths basically always turn toward the left, even if they're supposed to turn towards the right. A student made 5 paths (go straight, turn left, turn right, make clockwise circle, and drive a sine wave). They all turn left (or go in a counter-clockwise circle). Could there be a more fundamental problem?

icemannie commented 5 years ago

Did you invert the heading also? You need to.

On Sat, Jan 26, 2019, 8:01 PM William Rall <notifications@github.com wrote:

I'm not sure that the solution of swapping left and right paths is universally the case. I have some paths that work correctly and some that don't. My paths basically always turn toward the left, even if they're supposed to turn towards the right. A student made 5 paths (go straight, turn left, turn right, make clockwise circle, and drive a sine wave). They all turn left (or go in a counter-clockwise circle). Could there be a more fundamental problem?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wpilibsuite/PathWeaver/issues/137#issuecomment-457882256, or mute the thread https://github.com/notifications/unsubscribe-auth/ASWROlHtk-VRdnCeQzzcKg7O0_CG89zlks5vHQhngaJpZM4aLajM .

wrall commented 5 years ago

The paths generated for the below have basically the same right and left position values, turning left. The starting position for left and right are 0 inches, and the ending position for both paths are approximately 58" for left, 96" for right. image

I could swap left and right for one of them, but it would be useful to know when that might happen without inspecting the paths too closely.

Also, for one of the paths I generated which was supposed to drive straight forwards 48", the last position values generated are only 46.5".

kjrobrien commented 5 years ago

@wrall I cannot personally reproduce what you have described. With the current version of PathWeaver, the left and right paths are flipped when I create similar paths to the two you described. Could you please share your project with me? My instinct on the drive straight issue is that it MAY be related to Pathfinder itself, you may need to have a shorter time step (i.e. generate more points).

wrall commented 5 years ago

Smaller timestep does seem to allow it to reach the actual destination. Very strange behavior.

See paths below: PathWeaver.zip

kjrobrien commented 5 years ago

So, I admit that this is not straight-forward and we need to come up with a better way of signifying this: image But when I drag the path, I can see that what you "think" is the starting point, is actually the ending point. Therefore, it makes sense that the left and right do not have to be switched for this scenario, as the path as being configured is not correct. When I switch the starting and ending waypoints, I have the issue we have been finding. Calculating takes a decent amount of time. So when the paths are being dragged, we create an estimate of what the final path will be. Due to some idiosyncrasies with Pathfinder (our dependency for path generation), it created what looked like a valid path, but was actually the robot driving from near the cargo ship back to the HAB. Therefore, through my testing (I've created paths turning left and right from each quadrant of the field), I believe that the left and right sides do indeed need to be switched for all paths.

wrall commented 5 years ago

Ah, you're right - I assumed that the path was correct based on how it displayed, having not made them myself.

Some suggestions:

kjrobrien commented 5 years ago

I like all of these ideas! I've opened an issue for a feature request #138.