Skip to content
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

[cmd] After Command Decorators #7086

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

oh-yes-0-fps
Copy link
Contributor

Adds After and AfterTime to commands.

These are being upstreamed from choreolib CommandExt

This allows you to do trajCmd.after(gampiece().negate()) to wait to move until the robot shoots its gamepiece.

@oh-yes-0-fps oh-yes-0-fps requested review from a team as code owners September 16, 2024 02:25
This reverts commit 3c21556.
@oh-yes-0-fps
Copy link
Contributor Author

Ignore the WIP commit that got in by accident, oopsie.

Copy link
Contributor

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

@calcmogul calcmogul changed the title [wpilibNewCommands] After Command Decorators [cmd] After Command Decorators Sep 16, 2024
* @param condition the condition to wait for
* @return the decorated command
*/
public SequentialCommandGroup after(BooleanSupplier condition) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"after" might be a bit confusing, since we also have decorators that enforce command ordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally had waitFor but js suggested after, i can see both making sense

@KangarooKoala
Copy link
Contributor

KangarooKoala commented Sep 16, 2024

It might be clearer to implement these factories in terms of beforeStarting() (and optionally the corresponding Commands.wait() factory), especially for the C++ implementation.
For example:

 public SequentialCommandGroup after(BooleanSupplier condition) {
     // if we want this to bypass the `WaitUntilCommand` if the condition is already true,
     // we can use a conditional command but it proposes some compositional issues
-    return new SequentialCommandGroup(new WaitUntilCommand(condition), this);
+    return beforeStarting(Commands.wait(condition));
   }
 CommandPtr CommandPtr::After(std::function<bool()> condition) && {
-  AssertValid();
-  std::vector<std::unique_ptr<Command>> temp;
-  temp.emplace_back(std::make_unique<WaitUntilCommand>(std::move(condition)));
-  temp.emplace_back(std::move(m_ptr));
-  m_ptr = std::make_unique<SequentialCommandGroup>(std::move(temp));
-  return std::move(*this);
+  return std::move(*this).BeforeStarting(cmd::Wait(condition));
 }

oh-yes-0-fps and others added 3 commits September 22, 2024 21:41
Co-authored-by: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com>
Co-authored-by: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants