Alternative implementation of loop transitions

[rovo89]

Instead of finding a transition in the recorded polygon's points, look at the equally spaced points created from the polygon.

This fixes issues with sparse polygons (caused by map editing or with automatic point recording disabled). For dense polygons, which anyway have one point for every 10 cm, the result should be very similar as before.

[rovo89]

Based on #10.

Before:

After:

[rovo89]

I didn't test this in reality yet, but it looks good in the image. Maybe somebody already wants to have a look.

[rovo89]

Works fine with real grass as well ;)
Therefore I'd like to ask you to review and hopefully merge my changes, as a pre-requisite for ClemensElflein/open_mower_ros#56 and manually edited maps.

[rovo89]

Added a fix for the orientation of the last point per loop, see https://discord.com/channels/958476543846412329/961804411112394842/1267589322878746726 for visual comparison and the lengthy discussion with Pete_MI632 before.

[rovo89]

(the bug seems to have existed before any of my changes)

[ClemensElflein]

Thank you for the PR, due to extracting the loop in a separate functions, the changes are hard to see

Note that equally_spaced_points does not generate new points it will only try to sample equally spaced points with roughly the specified distance from the input polyline.

[rovo89]

Thank you for the PR, due to extracting the loop in a separate functions, the changes are hard to see

That's why I did the refactoring in a separate commit, which I even sent as a separate PR #10 in case there's disagreement on the logic to handle sparse maps 😉 But if you look at the individual commits in this PR, especially dfdf89f, it should be more obvious what the change idea for the sparse points is.

[rovo89]

Note that equally_spaced_points does not generate new points it will only try to sample equally spaced points with roughly the specified distance from the input polyline.

That contradicts my observations - I run a (partly) sparse map in production, and if the function didn't create new points, I would still see the problem that "2 points forward" sometimes means "several meters forward" (which is the main problem to be fixed in this PR).

Looking at the source code here and here, it definitely looks like it's interpolating to generate new points e.g. at half the distance between two input points to reach exactly the requested distance.

[ClemensElflein]

Yes you are right, thank you for linking the sources. Then my memory was just wrong there, I could have sworn that I had discussed this already with someone.

Then it makes sense to do it like that. I'll look at the code in depth and try the PR on some test maps

[rovo89]

Thanks for considering it! Maybe this helps in checking the impact of the changes: https://github.com/rovo89/open_mower_ros/commits/svg/. It's a first version of a script which creates an SVG from the planning results. Just need to start the slic3r via the included launch file. For me, that helped a lot to quickly iterate.

[ClemensElflein]

Nice, we could do a CI job which generates SVG for some example maps for each PR on this repo. This way we could quickly see if new code breaks something in (somewhat) complicated maps

[ClemensElflein]

looks good on my test maps, I'll merge this. I have merged #10, but it has conflicts with this one, so I'll just take this one instead since it seems to include changes in #10 anyways.

[rovo89]

looks good on my test maps, I'll merge this. I have merged #10, but it has conflicts with this one, so I'll just take this one instead since it seems to include changes in #10 anyways.

Ah, might be because you use squash-merging... I prefer rebase or merge commits because they keep the individual commits. As you noticed, looking at the overall diff can be overwhelming, but looking at the individual commits can make it much easier to follow the steps of the change. Depends on how the author works of course - if a PR has 10 commits with "changed this class" and "changed that other class", than there is less value in keeping them, but I tend to do e.g. refactoring first so that the actual change is better to see.

[rovo89]

Nice, we could do a CI job which generates SVG for some example maps for each PR on this repo. This way we could quickly see if new code breaks something in (somewhat) complicated maps

Yes, I think we briefly chatted on that topic. That would mean to include some example maps and the planner result in the repo. If some code change leads to a different result, the check would fail. If the change was intended, then include the new result in the commit. Although the script would need some more improvements for that, and probably the check should be done on some other textual representation to avoid breaking it when only a color changes or something. The SVG could still be included to visualize the change, as in the screenshots above.