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

Add zmp generator and populate all outputs of the unycycle trajectory generator #916

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

LoreMoretti
Copy link
Contributor

@LoreMoretti LoreMoretti commented Dec 10, 2024

Some of the outputs of the UnicycleTrajectoryGenerator were not being populated.

This PR fixes it.

Moreover, the ZMP generator (and its outputs) is added to the UnicycleTrajectoryGenerator

@LoreMoretti LoreMoretti changed the title Populate all outputs of the unycycle trajectory generator Add zmp generator and populate all outputs of the unycycle trajectory generator Dec 16, 2024
@LoreMoretti LoreMoretti force-pushed the unicycleTrajectoryGenerator/addOutputs branch from 03e1d0e to 7f78303 Compare December 18, 2024 15:32
@LoreMoretti LoreMoretti marked this pull request as ready for review December 18, 2024 15:32
@S-Dafarra
Copy link
Member

At the code level, it looks good. What I am a bit afraid of is that from outside it is not clear that the different trajectories are not coherent with each other. At the unicycle planner level, you are calling to different generators, so somehow you can expect them to be separate. Here instead, the output is in a single data structure. Maybe it would be useful to have a trigger to turn on or off this output?

@LoreMoretti
Copy link
Contributor Author

At the code level, it looks good. What I am a bit afraid of is that from outside it is not clear that the different trajectories are not coherent with each other. At the unicycle planner level, you are calling to different generators, so somehow you can expect them to be separate. Here instead, the output is in a single data structure. Maybe it would be useful to have a trigger to turn on or off this output?

You're right! It makes totally sense.

With the commit ecbd51f I tried to make the dcmTrajectory and the zmpTrajectory optional (and enabled by boolean flags).

I like the fact that std::optional makes the code cleaner in my opinion.

There's the drawback. This change makes the blf Unicycle Trajectory Generator not backward compatible. But I think the number of users is still low.

Let me know what you think.

@S-Dafarra
Copy link
Member

There's the drawback. This change makes the blf Unicycle Trajectory Generator not backward compatible. But I think the number of users is still low.

Let me know what you think.

It should be fine by changing the version number. On the other hand, another alternative is to apply this change only for the data you added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants