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

Remove the '/' from frame names #7

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

Conversation

corot
Copy link

@corot corot commented Jun 27, 2020

Can you create a melodic_dev branch, so I redirect this PR to it? Thanks!

@ipa-foj
Copy link
Contributor

ipa-foj commented Jun 28, 2020

Why do you want this to be merged in the original repository? In our case the "/" was required that's why it is written in the code

@corot
Copy link
Author

corot commented Jun 29, 2020 via email

@ipa-foj
Copy link
Contributor

ipa-foj commented Jun 29, 2020

Thanks for clarifying, I haven't worked with Melodic so far and didn't know that yet. I created a melodic_dev branch

@ipa-rmb
Copy link
Collaborator

ipa-rmb commented Jun 29, 2020

I would say it should work the same way in both versions. The "/" should not be needed or if needed, we should set it from outside as a parameter. Usually, the "/" should not be used explicitly. "/map" or "map" are application specific anyways, the map topic might be termed else by the user. Hence, having this set as a parameter or remapping from outside configuration files is a reasonable approach.

Maybe the problem described here arises from different system configurations and references to the "map" topic. I guess, it is not a problem between kinetic and melodic versions, but please correct me if I am wrong.

It would be advantageous not to create a separate melodic_dev version to avoid unecessary maintenance efforts for two almost similar versions. The raised problem should be likely handled the same way in kinetic and melodic.

@corot corot changed the base branch from indigo_dev to melodic_dev June 29, 2020 09:48
@corot
Copy link
Author

corot commented Jun 29, 2020

With TF2 (introduced with kinetic), frame ids shouldn't start with '/' (see the migration guide). But TF2 internally removes the slash, so most of the time it's not a problem. The only place where causes issues for me is here, cause the plan poses have '/map' as reference and move_base complains if the global frame is configured as 'map'.

So if you prefer, I can change the PR to set this as a parameter.

All that said, I think there are more things that break with melodic: so far I detected #6, plus another crash when trying to plan a route in a very small room (I didn't report yet). I suspect these issues are related to the use of OpenCV 3, instead of 2 (the default in indigo).... but I cannot confirm that easily. Would be great if someone still using both indigo and melodic can reproduce the issue!

@ipa-rmb
Copy link
Collaborator

ipa-rmb commented Jun 30, 2020

Alright, thanks for your comments. Then let's go with the new melodic branch as suggested. Seems to make more sense with a new branch if more problems are to appear.

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.

3 participants