[choreolib] Java triggers overhaul #589
Conversation
Removed old choreo command api
choreolib/src/main/java/choreo/trajectory/ChoreoTrajectory.java
Outdated
Show resolved
Hide resolved
|
It would be smart to add a wiki page on the pros and cons of triggers vs composition for autos. Also unsure if the trajectory factory should have methods that return commands directly and don't take AutoLoops to make composition feel more first class. // no need to make a loop or call `.cmd()`, the only issue is adding too many ways to do things and might confuse ppl
Command traj = factory.trajCmd("ampToMiddle"); |
oh-yes-0-fps
changed the title
I think TriggerExt should be upstreamed as part of Trigger or at least solid portions of it |
This was discussed in Discord and in some resolved convos in this thread, TriggerExt will not make it to release but can be left in for now to test better syntactic sugar that we can then upstream to wpilib. I think it should remain in for now and be removed later. |
| import java.util.Optional; | ||
|
|
||
| /** A trajectory loaded from Choreo. */ | ||
| public class ChorTrajectory<SampleType extends TrajSample<SampleType>> { |
There was a problem hiding this comment.
Can ChorTrajectory just be Trajectory? The package already disambiguates it. TrajSample should be TrajectorySample for clarity; letters are cheap.
There was a problem hiding this comment.
It being Trajectory was annoying because the intellisense would always put the wpilib trajectory above the choreo one so I would have to arrow key down twice to auto complete it.
There was a problem hiding this comment.
Alphabetical sorting over other things is a bad reason to choose the longer name. WPILib doesn't prefix all its classes with underscores to win the sorting race.
There was a problem hiding this comment.
Its not for alphabetical, its so you start typing it differently, starting with Chor will always pull it up. I agree with banks that its important for this one class. This class will also likely only be used by most users in one place, their trajectory logger lambda.
There was a problem hiding this comment.
The entire point of packages is to disambiguate names like this tho, so you can reuse good names from the namespace. Also, who's banks?
I would still like TrajSample renamed to TrajectorySample; it's a trajectory, not a "traj". Again, letters are cheap.
Those will come in a future pr, a majority of people use java so getting this out for user tests would be beneficial. |
| * ChorTrajectory}, {@link Boolean})->void, where the function consumes a trajectory and a | ||
| * boolean indicating whether the trajectory is starting or finishing. | ||
| * Trajectory}, {@link Boolean})->void, where the function consumes a trajectory and a boolean | ||
| * indicating whether the trajectory is starting or finishing. | ||
| */ | ||
| public interface TrajectoryLogger<SampleType extends TrajSample<SampleType>> |
There was a problem hiding this comment.
TrajSample should be TrajectorySample.
#469
I messed up and nuked the old pr by changing my forks branches around.
This pr is for the same features as the other.
There is currently only a Java implementation for the new API but if the interface is agreed upon I can also implement a python and c++ version.
I'm still working on a properly thought out example, I want to build it off of the rapid react command base example in wpilib.
for now you can look at my offseason code to get an idea of what the triggers API looks like.