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

Beginning goal id functions #2193

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

CursedRock17
Copy link
Contributor

This pull request is directly linked to issue #2073 which discusses the volubility of the the goal id in the async_send_goal. So far the thread has come to the conclusion that well-formed uuids should be implemented in accordance to RFC 4122 which may have been solved in pull request #1999, by converting to the appropriate string. Otherwise we must follow a representation of bit shifting seen in many examples.

Since the RFC issue may have already been addressed to formulate valuable UUIDs at version 4, I then added an overloaded function of async_send_goal which allows the user to add their own UUID. This is a double edged sword, depending on the way the user generates a UUID. Technically speaking, if the user has another UUID it could use a generator different from the one called in the method, so it is slightly dangerous to have both of these generators mix. This is only if the UUIDs end up being reserved already, but there so many combinations that this shouldn't happen, still a slight chance though. Conversely it can act very smoothly when the user continuesly uses their own generator for whatever reason they choose. In this case I feel that the unlikely chance of duplication in exchange for user comfortability justifies a overloaded method.

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@CursedRock17 CursedRock17 changed the title Beginning overloaded functions Beginning goal id functions May 17, 2023
@clalancette
Copy link
Contributor

We discussed this a bit, and we are not sure that this is a feature we should have. In particular, the GoalUUID is really an implementation detail of the actions, so supplying them from the outside doesn't seem to be something we should do.

Instead, if you really want a UUID that you have control of in your actions, then it makes more sense to add a UUID field to your action message. That way you'll have complete control of it, and not mess with the internal details of actions.

All of that said, we are still open to hearing more about the intended use case.

@clalancette clalancette added the more-information-needed Further information is required label May 25, 2023
@CursedRock17
Copy link
Contributor Author

CursedRock17 commented Jul 28, 2023

I'm not opposed to anything that you brought for keeping this method out of the library, I was just seeing if that implementation would be beneficial. We can close this PR and the issue attached if it's just stale and doesn't need anymore discussion.

On this note though, there might be an issue that still needs to be brought up: the goal id generation, where there were talks about doing something better for UUID generation. I don't know if this pull request was necessarily what they were looking for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants