Bug 511

Summary:	Need ability to change mobility model
Product:	ns-3	Reporter:	Gustavo J. A. M. Carneiro <gjcarneiro>
Component:	mobility models	Assignee:	Mathieu Lacage <mathieu.lacage>
Status:	RESOLVED FIXED
Severity:	normal	CC:	ns-bugs, raj.b
Priority:	P3
Version:	ns-3-dev
Hardware:	All
OS:	All
Attachments:	my current patch

Description Gustavo J. A. M. Carneiro 2009-02-25 06:47:26 UTC

Writing complex mobility models is not easy.  In my simulation of vehicles, I have a state machine for buses:

1. stopped
2. accelerate
3. constant velocity
4. decelerate
5. goto 1

It is much easier to just keep switching among several simple mobility model as the state changes, rather than write one big complex mobility model from scratch.

However, Object::AggregateObject does not allow changing one mobility model for another, and for good reason.  NetDevices could want to cache the mobility model, to avoid calling GetObject on a per packet basis, for instance.

In my tree I worked around this issue by hacking HierarchicalMobilityModel and make it accept a NULL parent.  This way, I can modify the Child mobility model as many times as I want, and with a NULL parent I made the model just return the absolute child position.

I could post these changes I made to that model, or we can come up with a new "container" mobility model to do the same.  I am ok either way, but the first case means one less class to worry about.

Comment 1 Mathieu Lacage 2009-04-16 05:12:40 UTC

Would you mind attach your patch for review ?

Comment 2 Gustavo J. A. M. Carneiro 2009-04-16 06:55:20 UTC

Created attachment 417 [details]
my current patch

Sorry, I have been having problems with codereview behind the company proxy, so here's the patch I use now.

Comment 3 Mathieu Lacage 2009-04-16 07:01:36 UTC

thanks, I see.

I think that the challenge with this approach is that it somewhat abandons an ns-3 invariant which is that, once created, the whole object topology does not change. Specifically, this means that the ConfigStore won't be able to capture the parameters of the models you change on the fly. Don't you have a per-node object which manages the mobility model changes ? If so, can't you just make this per-node object be a MobilityModel subclass ?

Comment 4 Gustavo J. A. M. Carneiro 2009-04-16 07:18:00 UTC

(In reply to comment #3)
> thanks, I see.
> 
> I think that the challenge with this approach is that it somewhat abandons an
> ns-3 invariant which is that, once created, the whole object topology does not
> change. Specifically, this means that the ConfigStore won't be able to capture
> the parameters of the models you change on the fly. Don't you have a per-node
> object which manages the mobility model changes ? If so, can't you just make
> this per-node object be a MobilityModel subclass ?
> 

I have such an object, but it is in Python, and for performance reasons it cannot directly be a mobility model.

This is not really topology information, IMHO.  The main point of HierarchicalMobilityModel is that you can have a node traveling inside a vehicular node, and I am putting this model to good use.  However, this HierarchicalMobilityModel is not very useful if a node is always attached to the vehicle and cannot leave the vehicle or switch to another vehicle.  I know you agree with me because the attributes Parent and Child had already existed before this patch :-)

My patch is only adding a special value to the Parent attribute.  Parent == NULL is currently invalid; with my patch, Parent == NULL means "take the Child mobility model as absolute coordinates, relative to the world (0,0,0)".

In other words, with the patch, Parent == NULL has the same effect as Parent == StaticMobilityModel(0,0,0), but is simpler and more efficient.

Comment 5 Mathieu Lacage 2009-04-16 07:27:28 UTC

(In reply to comment #4)

> > I think that the challenge with this approach is that it somewhat abandons an
> > ns-3 invariant which is that, once created, the whole object topology does not
> > change. Specifically, this means that the ConfigStore won't be able to capture
> > the parameters of the models you change on the fly. Don't you have a per-node
> > object which manages the mobility model changes ? If so, can't you just make
> > this per-node object be a MobilityModel subclass ?
> > 
> 
> I have such an object, but it is in Python, and for performance reasons it
> cannot directly be a mobility model.
> 
> This is not really topology information, IMHO.  The main point of

I said "object topology", not "network topology" and, by that, I meant: "the graph of pointer interconnections of C++ objects deriving from ns3::Object".

> HierarchicalMobilityModel is that you can have a node traveling inside a
> vehicular node, and I am putting this model to good use.  However, this
> HierarchicalMobilityModel is not very useful if a node is always attached to
> the vehicle and cannot leave the vehicle or switch to another vehicle.  I know
> you agree with me because the attributes Parent and Child had already existed
> before this patch :-)

Nice try, but, the reason why Parent and Child attributes where there was only to make is possible to control from ConfigStore and GtkConfigStore the attributes of the child and parent models. i.e., to read them. I made them settable because I am stupid.

> My patch is only adding a special value to the Parent attribute.  Parent ==
> NULL is currently invalid; with my patch, Parent == NULL means "take the Child
> mobility model as absolute coordinates, relative to the world (0,0,0)".
> 
> In other words, with the patch, Parent == NULL has the same effect as Parent ==
> StaticMobilityModel(0,0,0), but is simpler and more efficient.

I could be convinced to pull this in if you beef up the documentation :)

To summarize, that specific patch is ok if there is enough documentation but, the way you use this code in your tree is just plain evil and I hope no one ever gets the same idea :)

Comment 6 Gustavo J. A. M. Carneiro 2009-04-16 07:34:56 UTC

Well, if you think settable Parent and Child attributes are evil, then there is no point in the patch, documentation or not, is there?

Personally I don't think it's evil because I do not use ConfigStore.  But feel free to mark this WONTFIX if you think this is evil.

Comment 7 Mathieu Lacage 2009-04-16 07:43:43 UTC

ok, let's make a deal: if I take this patch, you become the maintainer of this class

Comment 8 Gustavo J. A. M. Carneiro 2009-04-17 06:20:20 UTC

I don't mind being maintainer of this class.  It's pretty simple code, actually.

Comment 9 Mathieu Lacage 2009-04-17 06:32:48 UTC

feel free to push your patch then