-
Notifications
You must be signed in to change notification settings - Fork 13
Modify RR vs Pedro into a table format #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Here are my thoughts.
First, I want to say that I completely agree with this. It's not only a hassle to duplicate every point (and doesn't add any value or new information for the reader), but can also lead to things that are misleading. For example, (as explained later) AdvantageScope support may be a pro of Roadrunner, but it is not a con of Pedro. Driving FasterWording less absolutely is good. This does the opposite of that. For example, changing from "can make your bot drive faster" to "makes your bot drive faster" is definitely not less absolutely. While I don't think saying "makes your bot drive faster" is good, I also don't like "can make your bot drive faster." This is because many good things about Pedro (and bad things about Roadrunner) are less definite, like "can make" and "potentially worse," where it's not true for the other side. I think the best option is to make that one "easier to make your bot drive faster." Although it is probably true that both can go that fast (although I'm not sure about that), it is likely much easier in Pedro. Less PeopleI think less people is a valid con and should not be removed. It is definitely something that could matter to many people, especially beginners. However, it is also true that while Pedro may have less, it is still very many. I think keeping it as "less people use it compared to Roadrunner" is completely fine, but possibly instead something like "less people use it compared to Roadrunner (although still a lot!)" Mostly ManualWhile it is true that Pedro tuning is more manual that Roadrunner's, I think "mostly manual" is false. It is unfair to count every single constant. A PID counts as one thing. Additionally, "32 manually tuned constants" is including ones that next to no one tunes. Even the Pedro developers. I think if you're going to decide how many things there are to tune you should have tuned Pedro before. I would count it like this:
While it is true that the manual tuners take longer than the automatic tuners, I would say time spent is roughly equal. Time ConsistencyI don't see why this matters. If it actually does and it's not just a buzzword, perhaps an explanation would help. Additionally, "speed is prioritized over consistency" is phrased like something bad, but going fast is good! Optimizing CurvesRoadrunner does not optimize the curves. LogsThe phrase "much more difficult debugging" cannot possibly be true. 99% of Roadrunner users do not use the logs for debugging. Additionally, it is extremely easy to log the pose yourself, if needed. I don't think this should be a con of Pedro, although being a pro of Roadrunner is completely fine. AdvantageScope"Full AdvantageScope support" is just a lie. The pull request has not been merged. Furthermore, even once it is merged, no AdvantageScope support is not a con of Pedro, just a pro of Roadrunner. I think removing it from Roadrunner pros is the right choice (as has been done in this PR) until the AdvantageScope PR is merged. It also looks like it was accidentally not removed from Pedro's cons. Bugs"Minimal bugs if any" is not true. Roadrunner has had its fair share of bugs recently. "Potentially buggier" was removed from Pedro's cons; I think this should be removed from Roadrunner's pros as well instead of adding "recently this has not been true." Neither is significantly buggy, and occasional bugs are expected. Bulk ReadsThis is not a con of Roadrunner 💀. CorrectionI think saying "potentially worse correction" is not true. Roadrunner has worse correction while following a trajectory. |
I think something like this might be better, and less susceptible to misleading.
Notice:
|
Some comments:
True. Also, speed means nothing if your auto is inconsistent.
This doesn't make sense. Trajectory and path following are two different things. Roadrunner has access to both Trajectory (the more commonly used variant) and Path (the lesser known but still existent and performant feature) following (along with a host of other capabilities and extensibility), whereas Pedro is exclusively a path follower. Putting them both as a path follower fundamentally misrepresents the capabilities of the two libraries.
This also doesn't make sense. Centripetal force correction only matters if you aren't motion planning, since it's essentially compensating for the path in front of you. Does roadrunner's path follower have centripetal force correction? Honestly, not sure, but it doesn't matter much. Roadrunner's trajectory follower definitely does, and that's the one people use. What do I mean by this? Well, Roadrunner uses motion planning to ensure the robot will (theoretically) track the path with equivalent accuracy to arc odometry (which i believe is convergent proportional to n^2). This compensates for curves by planning for the robot to travel around curves! If done correctly (can't comment on this as I haven't looked), this is actually a substantially better solution than just extrapolating off the current curvature.
The word "automatic" for RR here is a bit weird. The paths are still manually tuned, with automatic generation. Yes, it is more "automatic" than pedro's paths, but not really in a meaningful way. Users still manually tweak their paths in the same way you would tweak the control points in a pedro path. Suggested changes (something like this):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like this needs more eyes. From my perspective this PR has a lot of biases, but I am not impartial.
|
||
**Cons of Pedro:** | ||
- Newer, so potentially buggier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still true! (not as much as when this was first written, but that doesn't warrant taking it away).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the rewrite makes this much more the case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rewrite isn't public so it wouldn't make sense to discredit based on an unreleased version
- Newer, so potentially buggier. | ||
- Less people use it compared to Road Runner. | ||
- Tuning is mostly manual. <!-- 4 automatically tuned constants, minimum of 32 manually tuned constants --> | ||
- Not necessarily time-consistent; speed is prioritized over consistency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused why the line about time consistency was removed. This is necessarily true.
src/misc/pedro_vs_roadrunner.md
Outdated
- Time consistent by default. | ||
- Tuning is almost fully automated, making it difficult to screw up. <!-- 5 automatically tuned constants, 3 manually tuned constants --> | ||
<!--- Tuning is almost fully automated, making it difficult to screw up. 5 automatically tuned constants, 3 manually tuned constants ... PEOPLE still do? --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change seems reasonable, given influx of RR help requests. Comment is unnecessarily inflammatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change seems reasonable, given influx of RR help requests. Comment is unnecessarily inflammatory.
++. Please refrain from making such comments in cookbook. There have been 2 such comments in this PR.
Remove only the , difficult to screw up
part.
src/misc/pedro_vs_roadrunner.md
Outdated
- Uses the FIRST-recommended standard coordinate system consistently. | ||
- Works with SlothLoad immediately. | ||
- Bulk reads by default. | ||
<!-- - Works with SlothLoad immediately. || this is a quickstart thing, not a library thing itself? --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pedro was exclusively a quickstart before it was a library. The argument for this change seems moot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
atp both quickstarts both have the org.firstinspires.ftc.teamcode so 🤷🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, although not for the same reason, unless I'm understanding it wrong. I don't think it being a quickstart before a library matters. However, the quickstart is still very much part of Pedro Pathing. Until Pedro 1.1.0 releases (where this is fixed), it should be on there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to remove the sloth load line, next time please reference the change to the quickstart in the PR, especially as the change was made after the commits.
src/misc/pedro_vs_roadrunner.md
Outdated
@@ -70,7 +64,7 @@ Guidelines for editing this page: | |||
Differences must be objective issues from a neutral point of view, not one sided. | |||
Ideally, people with biases in both directions should agree about these differences. | |||
|
|||
Each difference will be listed twice, as a pro of one library and as a con of another. | |||
// Just because something is a pro of smth doesnt mean its a con of the other... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes seem ok. Comment is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also... why //
😭
It's already inside an HTML comment!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea that the comment promotes is fine, we should be clear about the idea that going forward, we should be careful about a pro of one library not being a con of another, but we should remove the ellipses at least, and it would be better to have this formally worded as part of an article prelude, as this sentence is very informal.
Oh also - would like to point out again that this
is meaningless buzzwords. What is a "reactive vector follower"? Call it a path follower T_T |
This is not true in RR 1.0. It does not have a path follower. I think it should stay how I wrote it (except change Path Following to just Following or possibly Following Strategy).
Yeah, I agree, I just left it how it was originally. It also does not determine the shape of the curve automatically in any way; that is completely manual. Maybe just do "cubic Bezier curve" and "quintic Hermite spline." |
I agree with this too. |
Can you please squash your three commits into one, or split them with different names if they are purposefully separate. Force push to the branch and update the PR. |
@Froze-N-Milk Done. |
Oops, clicked the wrong button. 💀 |
src/misc/pedro_vs_roadrunner.md
Outdated
| **Path Following** | Path Follower | Trajectory follower using motion profiling | **Visualizer** | Web-based using a nonstandard (0 - 144) coordinate scheme | Code-based using the standard FTC coordinate scheme | | ||
| **Tuning** | Half manual and half automatic | Nearly all automatic | | ||
| **Motor Write Caching** | Used by default | Able to be implemented manually | | ||
| **Paths** | Cubic Bezier curves | Quintic Hermite splines | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think this section is relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get 5, 6, 7 but I think 8 should probably stay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hex commented on 8 lol.
I think that it should stay because even though it's unlikely to influence someone's decision, it's still good to know and it can't hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hex commented on 8 lol.
Oh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think just listing the type of curves used offers no value without explaining their advantages/disadvantages
**Pros of Road Runner:** | ||
- Stable, minimal bugs if any. | ||
- Time consistent by default. | ||
- Tuning is almost fully automated, making it difficult to screw up. <!-- 5 automatically tuned constants, 3 manually tuned constants --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With what? The content being removed or the fact that it's being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With what? The content being removed or the fact that it's being removed?
with what is being said I presume
I think the table format is a good idea and generally gets rid of a lot of duplication. However, it seems like this PR has also removed a lot (or maybe all??) of the explanations for why the reader should care about these differences? I think it's important to keep those. I've made a release/readme for advantagescope RR support and it's usable today: http://github.com/j5155/advantagescope Will add some detailed comments as well. |
src/misc/pedro_vs_roadrunner.md
Outdated
to dynamically converge to the target position. | ||
| Feature | Pedro Pathing | Road Runner | | ||
|--------|--------|--------| | ||
| **Path Following** | Path Follower | Trajectory follower using motion profiling | **Visualizer** | Web-based using a nonstandard (0 - 144) coordinate scheme | Code-based using the standard FTC coordinate scheme | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this following difference means needs to still be explained. Something like "Path Follower; faster by default and always tries to get back to the path" and "Trajectory Follower with Motion Profiling; consistently timed path by default and tries to catch up when it gets behind".
The visualizer difference looks good, though the line break seems to be missing?
src/misc/pedro_vs_roadrunner.md
Outdated
| **Path Following** | Path Follower | Trajectory follower using motion profiling | **Visualizer** | Web-based using a nonstandard (0 - 144) coordinate scheme | Code-based using the standard FTC coordinate scheme | | ||
| **Tuning** | Half manual and half automatic | Nearly all automatic | | ||
| **Motor Write Caching** | Used by default | Able to be implemented manually | | ||
| **Paths** | Cubic Bezier curves | Quintic Hermite splines | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put back the original wording, or something close to the original wording here. The difference is important: Pedro paths can be modified arbitrarily using control points, but the control points have to be set manually. Roadrunner paths have automatically optimized curvature based on the tangents of each path, but manually changing the curve of the path is a little harder.
src/misc/pedro_vs_roadrunner.md
Outdated
| **Motor Write Caching** | Used by default | Able to be implemented manually | | ||
| **Paths** | Cubic Bezier curves | Quintic Hermite splines | | ||
| **Bulk Reads** | Able to be implemented manually | Used by default | | ||
| **Logs** | Not built in | Automatic logging of every run | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Replay is a better term then Log here, and I would still like the "readable with AdvantageScope" link that goes to https://github.com/j5155/AdvantageScope now that I actually have a release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(update: the PR is merged now and support is official)
@BaronClaps could you please make some of the requested changes? I'd love to get this article up to date now that 2.0 is out. |
Cleaned up some of the pros/cons so they’re worded less absolutely, especially for Pedro.
Being a pro for one isn’t an automatic con for the other.
Fixed the AdvantageScope and path optimization sections to match what’s true right now.