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.

Sign up for GitHub

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

Jump to bottom

Fixup CLI modifying trajectory parameters #1201

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

Fixup CLI modifying trajectory parameters #1201

wants to merge 3 commits into from

Conversation

pjreiniger
Copy link

Fixes: #1197
Fixes: #1049

I tried to take the relevant parts from the mega PR that said it should fix the CLI problems.

I am not super well versed in Rust, so there might be a more rust-ic way to do the Result thing.

I tested this by running the 2025.0.3 gui on our paths last year and this year, making a commit, and then running choreo-cli.exe --chor <file path> --generate --all-trajectory and verifying there were no changes. Our paths aren't complicated, basically just stop points and max velocity constraints, but it works for those and doesn't break any tests.

If something does break from this, I would say If you like it then you should have put a test on it

@github-actions github-actions bot added the component: backend Rust/Tauri backend label Feb 16, 2025
@pjreiniger
Copy link
Author

I'm pretty sure those CI failures are flakes; linux and windows built on my fork fine, and the mac one seems to be broken on the main branch and other PR's. I don't think my changes should have any direct affect on ChoreoLib

calcmogul
calcmogul requested changes Feb 22, 2025
View reviewed changes
@pjreiniger
Copy link
Author

After testing with some new paths we tried to generate, I've completely changed the strategy for this.

After some digging, the solution proposed in the mega PR and partially copied here is not enough to work for a trajectory that has just waypoints (generate has never been called in the GUI).

Unlike previously stated, the control flow for CLI and GUI generation is very different. When generating from the GUI, only partial results from the generate function are used, and then additional steps like stamping the snapshot and guessing control intervals are calculated. The CLI must be updated to follow this flow as well.

This seems very gross. One would think that calling generate would do all of the updates to the TrajectoryFile struct, and the UI would just show these updates instead of doing extra steps. However, as this is in the middle of the season and I think one of the maintainers should make such sweeping changes anyway, I went with the minimally invasive approach and updated the CLI tool to mimic the extra steps the gui does. Since the CLI is pretty hopelessly useless at the moment, I don't think this will break anyone's workflow, unlike making a change to the primary control flow.

This was tested by running a python script that creates a .traj with only waypoints and constraints in it. I then verified that the results from hitting "generate all" in the GUI matched the results of running the CLI on all the trajectories.

Once again, I will say that I'm probably doing some very un-rust like things and that should be looked at.

@github-actions github-actions bot added component: cli Command-line interface and removed component: backend Rust/Tauri backend labels Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calcmogul calcmogul calcmogul requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
component: cli Command-line interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Running CLI unlinks pose.heading variable CLI and GUI still produce different results in beta8
2 participants
@pjreiniger @calcmogul