stdr-simulator-ros-pkg / stdr_simulator

A simple, flexible and scalable 2D multi-robot simulator.
http://stdr-simulator-ros-pkg.github.io/
GNU General Public License v3.0
97 stars 66 forks source link

Change internal map topic from "map" to "stdr_map" #164

Open mehditlili opened 10 years ago

mehditlili commented 10 years ago

During the implementation of turtlebot_stdr simulator, there was no other way to make things work than changing the frame_id of the map to "world" (Because the frame_id map is actually representing the odometry frame and is not fixed) or running another map_server hosting the same map but with a different frame_id. I prefer the first solution as it consumes less ressources and I tested it on STDR, the frame_id doesn't seem to affect the good functionning of STDR.

czalidis commented 10 years ago

Are you sure that everything works fine after this change? As I can see, you have to change map_server.cpp:94 also for transformations to work properly. Although, your approach might work accidentally if your map origin is zero [0, 0, 0] (this is the case for all maps in stdr_resources).

Conceptually, I agree that it is better to name the frame world as it is referring to the map representing the environment. But I don't think that this would resolve naming conflicts when it comes to frame_ids. If you are using a slam package that produces a map that has a different frame as world, you would probably end up with the same issues.

I am aware that the way stdr handles frame_ids is problematic when it comes to simulating robots that are doing some kind of localization. Actually, the above mentioned frames should be for internal use, for geometric calculations by stdr, and not exposed to the end user.

I am currently working to find a solution to that issue, maybe using a different master for internal stdr stuff. In the meantime, renaming map frame_id to world shouldn't affect other things, if done properly. If that helps your work, I am willing to proceed with that change.

mehditlili commented 10 years ago

I think hardcoding the frame_id's is not a good solution. Why not do like in AMCL or Move_base and get all the needed parameters from am launch file? The slam package from the navigation stack in ROS has all parameters defined this way so it is possible to tell slam to use world as frame_id. And yeah having world instead of map as a frame_id would really help for my actual situation (just to avoid having to run an additional map server and use the internal stdr map server). If The only change to be done has to be in map_server and map_loader I will commit those changes to this pull request.

czalidis commented 10 years ago

Since that frame should only exist for internal stdr purposes there is no point to make it configurable, such as reading it from the param server.

If you are willing to proceed with the pull request, please make sure to test the functionality with different map origins. The changes that I suggested in my first comment should be applied also.

czalidis commented 10 years ago

One more thing. As you can see, all the launchers in stdr_launchers spawn a static_trasnform_publisher that publishes a transform between world and map (ex. server_with_map_and_gui.launch:7). You have to exclude that from the launchers in order to test the changes.

mehditlili commented 9 years ago

So what do you think of this last commit? I canceled other commits.

czalidis commented 9 years ago

I see that you finally didn't change the frame_id, only the topic name. Is there a reason behind that decision?

I agree, the name of the map topic was a bit confusing, stdr_map fits better even if it is not intended for the end user.

mehditlili commented 9 years ago

Yes I decided that using my own map server is better since you said that map_stdr is intended only for internal use. But I would suggest in the future to subscribe from stdr to a normal ros map_server.

czalidis commented 9 years ago

So the separate map server you are using, has a different frame_id than map? If not, the issues discussed above remain.

The reasoning behind the change of the topic name form map to stdr_map is to avoid naming conflicts with other packages. Doesn't the same apply for the frame_id?

In my opinion either we change both the topic name and the frame_id or none of them.

I don't quite get what you are suggesting, but if you are saying that stdr should use the standard implementation of map_server, thats not possible due to extra functionality that is needed. Of course stdr uses the map_server's underlying library for image loading.

etsardou commented 8 years ago

@czalidis what is the status of this PR? Should we merge it or..?

czalidis commented 8 years ago

This PR partially addresses one of the many problems we have, regarding the internal STDR topics. I was holding this back because I was looking for a more general solution. Keep in mind that what is proposed here can be also resolved with a simple topic remap.

In general this PR was very useful because of the conversation that took place and clarified certain things. It can either be closed or left open, only for other people to read this conversation, util a solid solution has been found.

etsardou commented 8 years ago

Ok, lets keep it open for reference, until we have a sound implementation.

corot commented 8 years ago

Not sure if this helps, but... the fast, easy way I found to make things work the normal ROS way, with localization, move_base, and so on, is:

It's dirty and names are inconsistent, but like this I can simulate navigation/localization same way as with the real robot, what is what many people wants of a 2D simulator!