Add pyviz module

[Tom Henderson, 2010-10-24 02:18:33]

+1 to add to ns-3 main tree; I frequently use pyviz.  I added to the ns-3.10
page.

One comment I had was about documentation/examples for how to use plugins, write
a new plugin, etc.?

Are there any known limitations to be documented?

I don't think that the long-term home for pyviz is src/contrib, but I would be
fine to merge it there for this release.

[gjcarneiro, 2010-10-25 10:31:25]

On 2010/10/24 02:18:33, Tom Henderson wrote:
> +1 to add to ns-3 main tree; I frequently use pyviz.  I added to the ns-3.10
> page.
> 
> One comment I had was about documentation/examples for how to use plugins,
write
> a new plugin, etc.?

I added some (very short) information about plugins in wiki page:
http://www.nsnam.org/wiki/index.php/PyViz#Plugins

Needs improvement, I know, but it's a start.

> 
> Are there any known limitations to be documented?

When I find them, I put them in the wiki page.

> 
> I don't think that the long-term home for pyviz is src/contrib, but I would be
> fine to merge it there for this release.

I am still not very sure about what exactly means contrib vs otherwise, to be
honest.  It's all a bit too vague.  Flow Monitor is also in contrib, and it
seems to be working fine.  I really don't care.

[Tom Henderson, 2010-10-25 14:08:34]

On 2010/10/25 10:31:25, gjcarneiro wro
> 
> I am still not very sure about what exactly means contrib vs otherwise, to be
> honest.  It's all a bit too vague.  Flow Monitor is also in contrib, and it
> seems to be working fine.  I really don't care.

In the past, we put things into contrib/ that we were not convinced was stable
API or that we were not sure we wanted to maintain.  For instance, the stats/
code was put there as experimental API/framework (and never was moved out of
there so far).  We also put some things in there that were "late merge" to
ns-3-dev during the release process, in case we wanted to tweak after the
release.

Other people have complained that the rules for src/contrib are not clear, and I
agree.  As previously mentioned on the list, we are experimenting with making
ns-3 more modular and one of the outcomes, I believe, is that we will have
different classes of models (such as "core ns-3", "contributed, maintained by
ns-3 project maintainers, but an optional model in the build", "contributed but
unmaintained by the project", etc.) and probably at that time we will try to
decide what to do with src/contrib.

[gjcarneiro, 2010-10-25 15:06:44]

On 2010/10/25 14:08:34, Tom Henderson wrote:
> On 2010/10/25 10:31:25, gjcarneiro wro
> > 
> > I am still not very sure about what exactly means contrib vs otherwise, to
be
> > honest.  It's all a bit too vague.  Flow Monitor is also in contrib, and it
> > seems to be working fine.  I really don't care.
> 
> In the past, we put things into contrib/ that we were not convinced was stable
> API or that we were not sure we wanted to maintain.  For instance, the stats/
> code was put there as experimental API/framework (and never was moved out of
> there so far).  We also put some things in there that were "late merge" to
> ns-3-dev during the release process, in case we wanted to tweak after the
> release.
> 
> Other people have complained that the rules for src/contrib are not clear, and
I
> agree.  As previously mentioned on the list, we are experimenting with making
> ns-3 more modular and one of the outcomes, I believe, is that we will have
> different classes of models (such as "core ns-3", "contributed, maintained by
> ns-3 project maintainers, but an optional model in the build", "contributed
but
> unmaintained by the project", etc.) and probably at that time we will try to
> decide what to do with src/contrib.

In that case, I think pyviz can move outside contrib, since I can continue to
maintain (if not develop) it for the time being.  As for API stability, I think
in the past NS-3 APIs have changed regardless of the API being in contrib or
not, so I don't agree that API stability as criteria makes a lot of sense.

Should I move pyviz to src then? src/visualizer?

[Tom Henderson, 2010-11-05 03:07:02]

+1 for prioritizing this for merge soon

there need to be some top-level confchecks in waf for missing packages.  My
build initially failed on python-rsvg and python-gnomedesktop.

This raises warning:
[ 628/1361] cxx: src/contrib/model-typeid-creator.cc ->
build/debug/src/contrib/model-typeid-creator_1.o
../src/contrib/pyviz.cc: In member function ‘void
ns3::PyViz::SimulatorRunUntil(ns3::Time)’:
../src/contrib/pyviz.cc:315: warning: ‘static void
ns3::Simulator::RunOneEvent()’ is deprecated (declared at
debug/ns3/simulator.h:124)

I lost track of the status of the above RunOneEvent() issue; what is the status?

One possible option would be to delay the pdf snapshot, since if you want to
capture the mouse-over text, you can't, presently.  But this is not a big deal
to support if difficult, since other screenshot tools do this.

Regarding where this module should live, how about src/tools/visualizer? 
(create a new "tools" directory, such as was done in ns-2)

http://codereview.appspot.com/2692041/diff/1/examples/wireless/wifi-simple-ad...
File examples/wireless/wifi-simple-adhoc-grid.cc (right):

http://codereview.appspot.com/2692041/diff/1/examples/wireless/wifi-simple-ad...
examples/wireless/wifi-simple-adhoc-grid.cc:241: Visualizer::Run ();
Is there a cleaner way to enable this in the long term, since I envision lots of
programs will want to have this option?  Such as make Visualizer a Simulator and
control it via a global value?

http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/doc/readm...
File src/contrib/visualizer/doc/readme.txt (right):

http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/doc/readm...
src/contrib/visualizer/doc/readme.txt:9: For more information, see
http://www.nsnam.org/wiki/index.php/PyViz
can this be converted to a doxygen header?

I think I mentioned earlier, some brief documentation on the existing plugins
and how to write new ones would be helpful.

http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/visualize...
File src/contrib/visualizer/visualizer/base.py (right):

http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/visualize...
src/contrib/visualizer/visualizer/base.py:32: ns3.LoopbackNetDevice:
NetDeviceTraits(is_virtual=True, is_wireless=False),
what is the difference between wireless=False and wireless=None?

http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/visualize...
src/contrib/visualizer/visualizer/base.py:35: ns3.BaseStationNetDevice:
NetDeviceTraits(is_wireless=True),
I ran into an issue demoing this that TapBridge was not enabled, so I enabled it
with "is_virtual=True".  

Suggest also to enable WiMAX, or are there lingering issues?

Again, documentation of pyviz might mention that if you add new net devices,
that you have to edit this file to enable them.

[Tom Henderson, 2010-11-05 03:09:06]

> > One comment I had was about documentation/examples for how to use plugins,
> write
> > a new plugin, etc.?
> 
> I added some (very short) information about plugins in wiki page:
> http://www.nsnam.org/wiki/index.php/PyViz#Plugins
> 
> Needs improvement, I know, but it's a start.
> 

Sorry, I missed this reply (new documentation) previously.

[gjcarneiro, 2010-11-05 14:47:26]

http://codereview.appspot.com/2692041/diff/1/examples/wireless/wifi-simple-ad...
File examples/wireless/wifi-simple-adhoc-grid.cc (right):

http://codereview.appspot.com/2692041/diff/1/examples/wireless/wifi-simple-ad...
examples/wireless/wifi-simple-adhoc-grid.cc:241: Visualizer::Run ();
On 2010/11/05 03:07:02, Tom Henderson wrote:
> Is there a cleaner way to enable this in the long term, since I envision lots
of
> programs will want to have this option?  Such as make Visualizer a Simulator
and
> control it via a global value?

I think this is a good idea and easy to implement for C++ programs.  For Python
simulations, on the other hand, it is tricky to implement without running a new
nested Python interpreter inside the existing one.  I have to think about it...

http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/doc/readm...
File src/contrib/visualizer/doc/readme.txt (right):

http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/doc/readm...
src/contrib/visualizer/doc/readme.txt:9: For more information, see
http://www.nsnam.org/wiki/index.php/PyViz
On 2010/11/05 03:07:02, Tom Henderson wrote:
> can this be converted to a doxygen header?
> 
> I think I mentioned earlier, some brief documentation on the existing plugins
> and how to write new ones would be helpful.

I prefer to write documentation in the wiki; it's much easier.  And since pyviz
has very little API, it's more like usage tips, I think doxygen does not make a
lot of sense.

If we end up having a "global" visualization flag, I guess that flag could go
into doxygen.

http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/visualize...
File src/contrib/visualizer/visualizer/base.py (right):

http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/visualize...
src/contrib/visualizer/visualizer/base.py:32: ns3.LoopbackNetDevice:
NetDeviceTraits(is_virtual=True, is_wireless=False),
On 2010/11/05 03:07:02, Tom Henderson wrote:
> what is the difference between wireless=False and wireless=None?

is_wireless=None is not really a valid value; should be True or False.  I guess
the argument default value should be False, or else raise an error if it is
None.

http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/visualize...
src/contrib/visualizer/visualizer/base.py:35: ns3.BaseStationNetDevice:
NetDeviceTraits(is_wireless=True),
On 2010/11/05 03:07:02, Tom Henderson wrote:
> I ran into an issue demoing this that TapBridge was not enabled, so I enabled
it
> with "is_virtual=True".  
> 
> Suggest also to enable WiMAX, or are there lingering issues?
> 
> Again, documentation of pyviz might mention that if you add new net devices,
> that you have to edit this file to enable them.

No WiMax issues that I'm aware of. I did test WiMax recently and it was working,
I guess I forgot to commit the NetDeviceTraits change.

In the future I guess we could let pyviz assume some default values, or move
those values into ns3::NetDevice (it has so many already, a couple more
shouldn't matter too much..)

[gjcarneiro, 2010-11-15 11:53:50]

On 2010/11/05 03:07:02, Tom Henderson wrote:
> +1 for prioritizing this for merge soon
> 
> there need to be some top-level confchecks in waf for missing packages.  My
> build initially failed on python-rsvg and python-gnomedesktop.

I can make these optional. One is for SVG node icon support, which can only be
enabled programatically in a Python script.  The other is just to get a GNOME
icon for a "screenshot" action; the button could become text-only.

> 
> This raises warning:
> [ 628/1361] cxx: src/contrib/model-typeid-creator.cc ->
> build/debug/src/contrib/model-typeid-creator_1.o
> ../src/contrib/pyviz.cc: In member function ‘void
> ns3::PyViz::SimulatorRunUntil(ns3::Time)’:
> ../src/contrib/pyviz.cc:315: warning: ‘static void
> ns3::Simulator::RunOneEvent()’ is deprecated (declared at
> debug/ns3/simulator.h:124)
> 
> I lost track of the status of the above RunOneEvent() issue; what is the
status?

Mathieu thinks I can avoid using it, but I don't find it so easy.  Maybe he'll
contribute a patch, some day ;-)

> 
> One possible option would be to delay the pdf snapshot, since if you want to
> capture the mouse-over text, you can't, presently.  But this is not a big deal
> to support if difficult, since other screenshot tools do this.
> 
> Regarding where this module should live, how about src/tools/visualizer? 
> (create a new "tools" directory, such as was done in ns-2)
> 

Agreed.  Moved to src/tools in my tree.


>
http://codereview.appspot.com/2692041/diff/1/examples/wireless/wifi-simple-ad...
> File examples/wireless/wifi-simple-adhoc-grid.cc (right):
> 
>
http://codereview.appspot.com/2692041/diff/1/examples/wireless/wifi-simple-ad...
> examples/wireless/wifi-simple-adhoc-grid.cc:241: Visualizer::Run ();
> Is there a cleaner way to enable this in the long term, since I envision lots
of
> programs will want to have this option?  Such as make Visualizer a Simulator
and
> control it via a global value?
> 

I've implemented something like this.  Now the Visualizer class is gone. 
Instead I created a wrapper for the DefaultSimulatorImpl class, which you can
enable with (e.g.):

./build/debug/examples/csma/csma-multicast
--SimulatorImplementationType=ns3::VisualSimulatorImpl


>
http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/doc/readm...
> File src/contrib/visualizer/doc/readme.txt (right):
> 
>
http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/doc/readm...
> src/contrib/visualizer/doc/readme.txt:9: For more information, see
> http://www.nsnam.org/wiki/index.php/PyViz
> can this be converted to a doxygen header?
> 
> I think I mentioned earlier, some brief documentation on the existing plugins
> and how to write new ones would be helpful.
> 
>
http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/visualize...
> File src/contrib/visualizer/visualizer/base.py (right):
> 
>
http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/visualize...
> src/contrib/visualizer/visualizer/base.py:32: ns3.LoopbackNetDevice:
> NetDeviceTraits(is_virtual=True, is_wireless=False),
> what is the difference between wireless=False and wireless=None?
> 
>
http://codereview.appspot.com/2692041/diff/1/src/contrib/visualizer/visualize...
> src/contrib/visualizer/visualizer/base.py:35: ns3.BaseStationNetDevice:
> NetDeviceTraits(is_wireless=True),
> I ran into an issue demoing this that TapBridge was not enabled, so I enabled
it
> with "is_virtual=True".  
> 
> Suggest also to enable WiMAX, or are there lingering issues?
> 
> Again, documentation of pyviz might mention that if you add new net devices,
> that you have to edit this file to enable them.

Next step is to work on the NetDevicesTrait issue.  I think it can assume some
default values.  However, it has to issue a warning, because default values are
often wrong...

[gjcarneiro, 2010-11-15 12:37:42]

The new patch set (2) changes the following:

1. There's no more Visualizer class, visualizer is started via command-line
option --SimulatorImplementationType=ns3::VisualSimulatorImpl.  Consequently,
examples are unchanged now;

2. gnomedesktop and rsvg modules optional;

3. NetDeviceTraits assumes default values for unknown netdevices, with a
warning;

4. If a plugin doesn't import, a warning is printed but otherwise the visualizer
goes on;

5. Visualizer path is now src/tools/visualizer.

[Tom Henderson, 2010-11-17 22:25:29]

On 2010/11/15 12:37:42, gjcarneiro wrote:
> The new patch set (2) changes the following:
> 
> 1. There's no more Visualizer class, visualizer is started via command-line
> option --SimulatorImplementationType=ns3::VisualSimulatorImpl.  Consequently,
> examples are unchanged now;

this looks good; in the future, one could envision a "./waf --visualize
<program>" option that set the variable automatically, but I think that users
can implement their own shell tricks for now.

> 
> 2. gnomedesktop and rsvg modules optional;
> 
> 3. NetDeviceTraits assumes default values for unknown netdevices, with a
> warning;
> 
> 4. If a plugin doesn't import, a warning is printed but otherwise the
visualizer
> goes on;
> 
> 5. Visualizer path is now src/tools/visualizer.

I would say "merge now" but it is still dependent on deprecated RunOneEvent() so
it seems like that should be resolved one way or another.

[Tom Henderson, 2010-11-17 22:27:03]

I also tried this code on several examples and noticed that it didn't launch a
visualizer on this example, for some unknown reason:

./waf --run "wifi-hidden-terminal
--SimulatorImplementationType=ns3::VisualSimulatorImpl"

[gjcarneiro, 2010-11-17 23:59:23]

On 2010/11/17 22:25:29, Tom Henderson wrote:
> On 2010/11/15 12:37:42, gjcarneiro wrote:
> > The new patch set (2) changes the following:
> > 
> > 1. There's no more Visualizer class, visualizer is started via command-line
> > option --SimulatorImplementationType=ns3::VisualSimulatorImpl. 
Consequently,
> > examples are unchanged now;
> 
> this looks good; in the future, one could envision a "./waf --visualize
> <program>" option that set the variable automatically, but I think that users
> can implement their own shell tricks for now.

+1

> 
> > 
> > 2. gnomedesktop and rsvg modules optional;
> > 
> > 3. NetDeviceTraits assumes default values for unknown netdevices, with a
> > warning;
> > 
> > 4. If a plugin doesn't import, a warning is printed but otherwise the
> visualizer
> > goes on;
> > 
> > 5. Visualizer path is now src/tools/visualizer.
> 
> I would say "merge now" but it is still dependent on deprecated RunOneEvent()
so
> it seems like that should be resolved one way or another.

I don't have time or motivation to work on that issue.

[gjcarneiro, 2010-11-18 00:00:24]

On 2010/11/17 22:27:03, Tom Henderson wrote:
> I also tried this code on several examples and noticed that it didn't launch a
> visualizer on this example, for some unknown reason:
> 
> ./waf --run "wifi-hidden-terminal
> --SimulatorImplementationType=ns3::VisualSimulatorImpl"

The reason is that this example does not call CommandLine::Parse, so it ignores
the --SimulatorImplementationType option.

[Mathieu Lacage, 2010-11-18 08:23:13]

I did not have time to go through a lot of code but it all looks nice. However,
I am concerned by the lack of doxygen (the fact that you don't like it is not
sufficient reason to not write doxygen doc) and I really don't understand why
you still call RunOneEvent. 

From my perspective, both are requirements for merging:
  - you can't add new code in ns-3 that uses deprecated APIs
  - you can't add new code in ns-3 that does not have doxygen doc

as-is, -1 for merging.

http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/p...
File src/tools/visualizer/model/pyviz.cc (right):

http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/p...
src/tools/visualizer/model/pyviz.cc:255: m_stop = true;
call Simulator::Stop here.

http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/p...
src/tools/visualizer/model/pyviz.cc:315: Simulator::RunOneEvent ();
I really don't understand why you can't call Simulator::Run here and call ::Stop
when you need to make it return to you.

http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/v...
File src/tools/visualizer/model/visual-simulator-impl.cc (right):

http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/v...
src/tools/visualizer/model/visual-simulator-impl.cc:43: m_defaultSim =
CreateObject<DefaultSimulatorImpl> ();
add an attribute of type ObjectFactory that defaults to DefaultSimulatorImpl so
that we allow other simulators to be used.

http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/v...
File src/tools/visualizer/model/visual-simulator-impl.h (right):

http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/v...
src/tools/visualizer/model/visual-simulator-impl.h:31: class VisualSimulatorImpl
: public SimulatorImpl
missing doxygen. please, add, explain what this does.

[gjcarneiro, 2010-11-18 10:52:52]

On 2010/11/18 08:23:13, Mathieu Lacage wrote:
> I did not have time to go through a lot of code but it all looks nice.
However,
> I am concerned by the lack of doxygen (the fact that you don't like it is not
> sufficient reason to not write doxygen doc) and I really don't understand why
> you still call RunOneEvent. 
> 
> From my perspective, both are requirements for merging:
>   - you can't add new code in ns-3 that uses deprecated APIs
>   - you can't add new code in ns-3 that does not have doxygen doc
> 
> as-is, -1 for merging.
> 
>
http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/p...
> File src/tools/visualizer/model/pyviz.cc (right):
> 
>
http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/p...
> src/tools/visualizer/model/pyviz.cc:255: m_stop = true;
> call Simulator::Stop here.
> 
>
http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/p...
> src/tools/visualizer/model/pyviz.cc:315: Simulator::RunOneEvent ();
> I really don't understand why you can't call Simulator::Run here and call
::Stop
> when you need to make it return to you.

Because then I don't know, when Simulator::Run returns, wheter the simulation
has stopped because it's finished or because we asked it to. 
Simulator::IsFinished() returns true in both cases.

> 
>
http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/v...
> File src/tools/visualizer/model/visual-simulator-impl.cc (right):
> 
>
http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/v...
> src/tools/visualizer/model/visual-simulator-impl.cc:43: m_defaultSim =
> CreateObject<DefaultSimulatorImpl> ();
> add an attribute of type ObjectFactory that defaults to DefaultSimulatorImpl
so
> that we allow other simulators to be used.
> 

OK, I guess.  But I can't imagine pyviz working with anything but the default
scheduler, atm...

>
http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/v...
> File src/tools/visualizer/model/visual-simulator-impl.h (right):
> 
>
http://codereview.appspot.com/2692041/diff/13001/src/tools/visualizer/model/v...
> src/tools/visualizer/model/visual-simulator-impl.h:31: class
VisualSimulatorImpl
> : public SimulatorImpl
> missing doxygen. please, add, explain what this does.

This class is private; do we really need to document it?  This is not API that
is meant to be used. Same thing for the PyViz class. The PyViz class is public
only because I need Python bindings for it.  Otherwise, it is private for the
visualizer tool.

[gjcarneiro, 2010-11-21 22:37:54]

1. add ObjectFactory;

2. don't use Simulator::RunOne (at the cost of not knowing when the simulation
should stop, now pyviz just runs forever)

3. add one line doxygen comments stating the pyviz APIs are internal, add them
to the doxygen ignore list

[Tom Henderson, 2010-11-22 04:59:02]

> 3. add one line doxygen comments stating the pyviz APIs are internal, add them
> to the doxygen ignore list

FWIW, I suggested this to Gustavo; I am fine with this doxygen solution in this
particular case.

[Mathieu Lacage, 2010-11-22 19:37:41]

On 2010/11/18 10:52:52, gjcarneiro wrote:
> > I really don't understand why you can't call Simulator::Run here and call
> ::Stop
> > when you need to make it return to you.
> 
> Because then I don't know, when Simulator::Run returns, wheter the simulation
> has stopped because it's finished or because we asked it to. 
> Simulator::IsFinished() returns true in both cases.

I would have expected IsFinished to return true only if there are no more
events. Maybe we should introduce GetNRemainingEvents () or HasMoreEvents or
something like that then (pick a better name if you can).

> > CreateObject<DefaultSimulatorImpl> ();
> > add an attribute of type ObjectFactory that defaults to DefaultSimulatorImpl
> so
> > that we allow other simulators to be used.
> > 
> 
> OK, I guess.  But I can't imagine pyviz working with anything but the default
> scheduler, atm...

How come ? If it uses the public API, why would it be a problem ? Do we have
bugs in the realtime scheduler implementation that make it incompatible with
pyviz ? (other than the runone crap) if so, please, report them.

> > src/tools/visualizer/model/visual-simulator-impl.h:31: class
> VisualSimulatorImpl
> > : public SimulatorImpl
> > missing doxygen. please, add, explain what this does.
> 
> This class is private; do we really need to document it?  This is not API that
> is meant to be used. Same thing for the PyViz class. The PyViz class is public
> only because I need Python bindings for it.  Otherwise, it is private for the
> visualizer tool.

It's public because you expect people to use the name from the command-line.
From that perspective, you need to at least document in the header what it does
and what you can do with it. i.e., the doxygen should minimally hint at how you
use it from the command-line if only for those who read this code later and have
no idea where it's coming from.

[gjcarneiro, 2010-11-22 19:55:47]

On 2010/11/22 19:37:41, Mathieu Lacage wrote:
> On 2010/11/18 10:52:52, gjcarneiro wrote:
> > > I really don't understand why you can't call Simulator::Run here and call
> > ::Stop
> > > when you need to make it return to you.
> > 
> > Because then I don't know, when Simulator::Run returns, wheter the
simulation
> > has stopped because it's finished or because we asked it to. 
> > Simulator::IsFinished() returns true in both cases.
> 
> I would have expected IsFinished to return true only if there are no more
> events. Maybe we should introduce GetNRemainingEvents () or HasMoreEvents or
> something like that then (pick a better name if you can).

I guess.  HasMoreEvents sounds good.  But I don't think it should block this
issue; it can be fixed later.

> 
> > > CreateObject<DefaultSimulatorImpl> ();
> > > add an attribute of type ObjectFactory that defaults to
DefaultSimulatorImpl
> > so
> > > that we allow other simulators to be used.
> > > 
> > 
> > OK, I guess.  But I can't imagine pyviz working with anything but the
default
> > scheduler, atm...
> 
> How come ? If it uses the public API, why would it be a problem ? Do we have
> bugs in the realtime scheduler implementation that make it incompatible with
> pyviz ? (other than the runone crap) if so, please, report them.

Let's just say I'm not confident that the realtime scheduler will run in as
real-time as expected under pyviz, under the stop-go-stop-go-stop-go-... regime.
 But I haven't tested it.  If it works, great, but I don't think it matters.

> 
> > > src/tools/visualizer/model/visual-simulator-impl.h:31: class
> > VisualSimulatorImpl
> > > : public SimulatorImpl
> > > missing doxygen. please, add, explain what this does.
> > 
> > This class is private; do we really need to document it?  This is not API
that
> > is meant to be used. Same thing for the PyViz class. The PyViz class is
public
> > only because I need Python bindings for it.  Otherwise, it is private for
the
> > visualizer tool.
> 
> It's public because you expect people to use the name from the command-line.
> From that perspective, you need to at least document in the header what it
does
> and what you can do with it. i.e., the doxygen should minimally hint at how
you
> use it from the command-line if only for those who read this code later and
have
> no idea where it's coming from.

Yes, there's a minimal documentation saying just that, if you check in the
patch.  But I don't want to document the other methods.

[gjcarneiro, 2010-12-06 18:00:20]

Merged!

changeset:   6676:8a57344a8d09
user:        Gustavo J. A. M. Carneiro  <gjc@inescporto.pt>
date:        Mon Dec 06 17:26:06 2010 +0000
summary:     Codereview Issue 2692041: Add pyviz module

changeset:   6677:06864ef8c0a1
tag:         tip
user:        Gustavo J. A. M. Carneiro  <gjc@inescporto.pt>
date:        Mon Dec 06 17:59:20 2010 +0000
summary:     Python bindings rescan for pyviz