simspark-devel — Discussion about Spark development

[simspark-devel] rcssserver3D release

[Oliver O. <fr...@tz...>]

Dear all,

(please let me know if I forgot to include somebody who is developing)

I have checked in the source code I got from you guys so far for the 
rcssserver3D release. I guess what is missing now is the changes in the 
VisionPerceptor (Jan), and the new timing model (Rujia). 

I think we can proceed as follows:

* For the release candidate on Wednesday: 
  (release with all new features but spades timing)

- Jan: please check in the visionperceptor changes as soon as your done 
  (preferable on Tuesday). 
- (Hesham):
  you mentioned that you could fix some of the reported bugs (regarding 
  the Random Number Generators). If you find the time to do that, please 
  do that (I'll also try to fix some bugs tonight). 
- (Markus):
  I know you're busy but do you have a nice screenshot of one of the more 
  complicated robots(+rsgedit) at hand? Would be quite useful for   
  explaining the roadmap :) 
- all: please test the version in the CVS now, and report / fix possible
       showstoppers (no new features, please).

* For the final release:

- Rujia: please tell me the status of your work (replace timing).
  
- all: depending on the status of Rujias work, we all have to work on the 
       timing. (Sorry but it's really important to change it).
       This should be done by March 14 at the latest.
- all: fixing reported bugs

* For RoboCup, we're planning to move the soccer simulation completely to 
  simspark repository, so that we can introduce everything new for 2007 on 
  RoboCup 2006. Any help is highly appreciated :-)

cheers
Oliver

-- 
Oliver Obst                     form follows function. Louis Sullivan
Uni Bremen    +49 421 218 4676
Intelligent Systems Department      Center for Computing Technologies

Re: [simspark-devel] Re: [Sserver-three-d]Agent Say- and Heareffector

[Markus R. <rol...@un...>]

Hi,

Hesham wrote:
> I'm testing google cpu profiler (
[...]

I just read about google profiler. It looks nice, especially the 
graphical output of the call graph.

> Can I know your opinion about code optimization? do you use any tools
> to analysis the performance of the server?

In general profiling can be a valuable tool if used with caution. But I 
think that readable and maintainable code that is well structured is 
more important than squeezing out the last bit of performance. This is 
especially true if more than one developer is working on a piece of code 
  in an open source project.

The brainstormers 3d team (Germany, Karlsruhe) used a profiler on the 
server and found that most time was spent resolving path names (e.g. 
/sys/server/input) to pointers. Therefore we implemented a node cache to 
speed up the server. So in this case you don't profile a specific 
section of your code (like your offside rule) but try to find out where 
the global bottleneck is.

It would be interesting to know where the soccer simulation spends most 
of its time, i.e. the soccer plugin. Is it possible to limit profiling 
to this plugin?

Another thing: It would be nice if you could document your profiling, 
say/hear  and offside rule work. I.e. how to setup and use the google 
profiler along with a summary of your results. And for the plugins a 
kind of todo/done list.

For the simspark/soccer3d project Oliver already setup a wiki at [1] 
(the editing password is 'spark'). It's intended as a shared todo list 
and documentation wiki if you like.

regards,
Markus

[1] http://simspark.sourceforge.net/pmwiki/

[simspark-devel] Re: [Sserver-three-d]Agent Say- and Heareffector

[Hesham <hes...@gm...>]

Hi Markus

On 12/29/05, Markus Rollmann <rol...@un...> wrote:
>
> Hi Hesham,
>
> Hesham wrote:
> > Sorry for this mistake.I saw this bug, it was my mistake when
> > changing the code style I made this mistake.
>
> that's no problem... You're not the first programmer to make a mistake ;-=
)
>


Thanks, anyway I'll try to not repeat the mistake :-)

> I changed Broadcast() in soccerruleaspect.cpp and attached it, could
> > you please update the CVS?
>
> Thanks for your fast reply and good work. I'll update the CVS as I find
> the time.
>
> > BTW one of my friends is working on testing communication. After his
> >  tests I'll make the needed changes and send the new communication
> > code.
>
> I think Joschka plans a new server release for the end of this month and
> another one before the next competition. Maybe you can coordinate with
> him which features you can include in each version?


Sure, thanks, I'll be in contact with Joschka.

I'm testing google cpu profiler (
http://goog-perftools.sourceforge.net/doc/cpu_profiler.html) on the 3D
server, cuz I think it's better after making the communication and offside
code bug free (at least at an acceptable level), I start to optimize these
codes specially the offside code. Can I know your opinion about code
optimization? do you use any tools to analysis the performance of the
server?

Thanks,
Hesham

Re: [simspark-devel] Re: communication code progress

[Markus R. <rol...@un...>]

Hi,

sorry for the long delay- I'm quite busy at the moment. I just committed 
Heshams code to the CVS and added him to the THANKS file.

Hesham wrote:
> ifText and GetText are using in sexpmonitor.cpp which adds said messages 
> to monitor infos. In my tests it worked without problem.
> 
> (Info (P (s 1)(id 1)(pos -50 0 0.22)(say Test_1))(P (s 1)(id 2)(pos 
> -37.9934 0.00654434 0.22))(B (pos 0 0 0.111)))

It seems that ifText is used as a flag that tells the Monitor, that the 
SayEffector has been used. However

- The naming isn't obvious
- The flag shouldn't be part of an effector. An effector is not supposed 
  to hold any state (neither ifText nor mMessage)
- The flag is never reset

I propose the following:
- Remove the flag and mMessage from Sayeffector and move it into 
Agentstate.
- Change the flag to a time stamp (read the current time from 
GameStatAspect).
- In the sexpmonitor compare the current time to the time stamp to check 
if a player said something since last update

Any volunteers ? ;)

regards,
Markus

Re: [simspark-devel] Re: communication code progress

[Joschka B. <jbo...@un...>]

Hi,

Markus Rollmann wrote:
> Hi,
>
> Hesham wrote:
> > Thank you so much for your fast and complete review. My code had a lot
> > of coding style mistakes, if I know this I didn't send it :-)
>
> I think your code was actually quite ok. It's just that every project
> uses it's own style of coding in order to make life easier for the
> developers as the need to read each other's code. Otherwise the codebase
> would result in a mixture of different styles. The only trouble is that
> most projects use their very own style guide. So sending in you're code
> for review is a good way to adapt to it ;-)
>
> >     - What is the meaning of the 'ifText' member? Please add a comment to
> >     sayeffector.h explaining it
> >
> >     - All member variables should start with a small 'm' for
> >     'member'. Therefore the ifText member should be renamed 'mIfText'
> >
> >
> > But I didn't write it. It seems it's a variable that sexpmonitor uses
> > that. But I didn't change it because I'm not sure about it.
>
> Ok I see. It seems that Irene Markelic committed that code... (according
> to 'cvs annotate sayeffector.h') But now you're our expert for the
> sayeffector ;-)
>
> Do you understand how ifText is used in the sayeffector? Can you come up
> with a better name for it? Or can we safely remove this code?

I don't get the meaning of this method either. It seems to be something very 
specific that Irene implemented for her undergrad thesis. 

@Oli: do you remember anything about that?

> I looked through the changed version of you're code and I think it looks
> very good now. If Joschka and Oliver are Ok with it I think we can
> commit it.

It looks very good. Just a small little thing in the 
SayEffector::GetActionObject() method:

The do...while loop is not necessary, I think. It's just run one time anyways, 
so you could just move the construction of the empty ActionObject into the 
if-statement where you check for the message error:

if (predicate.name != GetPredicate())
{
    GetLog()->Error() << "ERROR: (SayEffector) invalid predicate"
                      << predicate.name << "\n";
    break;
}

Predicate::Iterator iter = predicate.begin();

std::string message;
if (! predicate.AdvanceValue(iter, message))
{
    GetLog()->Error()
        << "ERROR: (SayEffector) said message expected\n";

    // some error happened
    return shared_ptr<ActionObject>();
}

// construct the SayAction object
return shared_ptr<SayAction>(new SayAction(GetPredicate(), message));

> I think there is just one thing missing: In order to use the say
> effector with the soccer simulation you had to modify the
> rcssserver3d.rb (app/simulator/rcssserver3d.rb, function addagent) to
> install the effector. I think you missed to include this change in the
> file you sent.

Right. If possible, please send this file for reference.

Cheers,
Joschka

[simspark-devel] Re: communication code progress

[Markus R. <rol...@un...>]

Hi,

Hesham wrote:
> Thank you so much for your fast and complete review. My code had a lot 
> of coding style mistakes, if I know this I didn't send it :-)

I think your code was actually quite ok. It's just that every project 
uses it's own style of coding in order to make life easier for the 
developers as the need to read each other's code. Otherwise the codebase 
would result in a mixture of different styles. The only trouble is that 
most projects use their very own style guide. So sending in you're code 
for review is a good way to adapt to it ;-)

>     - What is the meaning of the 'ifText' member? Please add a comment to
>     sayeffector.h explaining it
> 
>     - All member variables should start with a small 'm' for
>     'member'. Therefore the ifText member should be renamed 'mIfText'
> 
>  
> But I didn't write it. It seems it's a variable that sexpmonitor uses 
> that. But I didn't change it because I'm not sure about it.

Ok I see. It seems that Irene Markelic committed that code... (according 
to 'cvs annotate sayeffector.h') But now you're our expert for the 
sayeffector ;-)

Do you understand how ifText is used in the sayeffector? Can you come up 
with a better name for it? Or can we safely remove this code?

I looked through the changed version of you're code and I think it looks 
very good now. If Joschka and Oliver are Ok with it I think we can 
commit it.

I think there is just one thing missing: In order to use the say 
effector with the soccer simulation you had to modify the 
rcssserver3d.rb (app/simulator/rcssserver3d.rb, function addagent) to 
install the effector. I think you missed to include this change in the 
file you sent.

regards,
Markus

[simspark-devel] Re: communication code progress

[Markus R. <rol...@un...>]

Hi Hesham,

I looked over the code you've submitted and I think it looks okay and
it compiled without errors (gcc 3.3.4).

However I've got some comments about the coding style that I ask you
to change. The comments below are about how the 'perfect' code in our
CVS should look- you'll certainly find many places that differ from
this style. But we should try to minimize these ;-)

In the doc/ section of the CVS you'll find a document called
codingstyle.txt that explains this in detail.

regards,
Markus

SayAction.h
-----------

void GetMessage(std::string& mess)
- call it 'msg' instead of 'mess' ;-)

- is the #include <salt/vector.h> necessary =

sayeffector.h
-------------

- Are body.h and ball.h #includes necessary?

- Please try to minimize number of #includes in .h files as they
increase the compile time and number of dependencies in large
projects. All class types that you only need in pointer types or
references can be forward declared. Move the #include statements into
the .cpp file.

Example:

--(instead of )--

#include <oxygen/agentaspect/agentaspect.h>
#include <soccer/soccerruleaspect/soccerruleaspect.h>

---

-- (write) --

namespace oxygen
{
	class AgentAspect;
	// other forward declarations...
}
class SoccerRuleAspect;

---

sayeffector.cpp
---------------

- unnecessary #include: random.h

- What is the meaning of the 'ifText' member? Please add a comment to
sayeffector.h explaining it

- All member variables should start with a small 'm' for
'member'. Therefore the ifText member should be renamed 'mIfText'

hearperceptor.cpp
-----------------
- ther are left over commented lines (87-89)

agentstate.h
------------

- Please add some comments to the new member variables that describe
their meaning (mHearMax, mHearInc etc.)

agentstate.cpp
--------------

- please indent blocks under if, for and while statements, i.e. add {
} lines: (line 125, 126 and 134, 135 and 184,185 )

if (mHearMateCap < mHearDecay)
{ // add these lines
     return;
}

- line 134: please put opening and closing brackets on separate lines
and indent the following block

         // end of block1
     } else
         {
             // start of block2

soccerruleaspect.cpp
--------------------
- please indent blocks under if statements, i.e. add { } lines (same
as in agentstate.cpp)

- don't declare a loop iterator outside of the loop. This is bad style
and can lead to subtle errors (e.g. unintended reuse). Please declare
it inside the loop, i.e. declare it in line 733 and 756.

- typedef complex type, i.e. don't write
    std::list<boost::shared_ptr<AgentState> >::const_iterator i;

    instead write something like
    typedef std::list<boost::shared_ptr<AgentState> > TAgentStateList
    in a .h file

    then write only TAgentStateList::const_iterator in the .cpp- thats
    far more readable. This typedef could go into soccerbase.h (you
    could modify the GetAgentStates function to use this type when
    you're at it). Someone made the same mistake there and used a very
    very long type in the function instead of a typedef

    You'll find an example for this method is in
    lib/zeitgeist/leaf.h. There (line 57) TLeafList is declared and
    used for example in GetChildren (line 136)

- please dont't use std:: and boost::, oxygen::, salt:: etc. prefixes
in .cpp files. That's harder to read. At the beginning of
soccerruleaspect.cpp you'll see 'using namespace' statements (line
34,35). This means that you can ommit these prefixes and write
e.g. 'list' instead of 'std::list'. Note: Only use 'using namespace'
in .cpp files.

- futher please format complex and long for statements over 3 lines,

Your for loops should then look something like this:

      for (
	  TAgentStateList::const_iterator it = agent_states.begin();
	  it != agent_states.end();
	  ++it
          )
	 {
	  // ...
          }

Re: [simspark-devel] communication code progress

[Markus R. <rol...@un...>]

Hi,

Hesham wrote:
> Almost the communication code is ready. I attached the latetst
> version, that is similar to say and hear in the 2D soccer server
[...]

thanks for your work! I didn't have time to review your code in detail 
yet (and i'm not near a linux box atm), so I'll comment in a separate 
message later.

[...]
> I used SoccerRuleAspect because it has easy access to all players.
> But I guess for spark I should change it, but which class is better?

I think it's ok to use the SoccerRuleAspect for message dispatching. 
We're trying to adapt it for the spark soccer version anyhow. Further 
its' the right place for policy decisions of all sorts, i.e. filtering 
what a specific player is allowed to hear etc.

Note that the SoccerRuleAspect is currently disabled in the spark soccer 
version, so the say/hear infrastructure won't work.

>  unfortunately I couldn't run spark (cvs version), this is its error:
>  ./simspark.rb: line 11: syntax error near unexpected token 
> `sparkAddFPSCamera' ./simspark.rb: line 11: `sparkAddFPSCamera('

the sparkAddFPSCamera function is defined in lib/spark/spark.rb. Have 
you installed spark? (i.e run 'make install in the lib/ directory or 
directly in lib/spark). This should copy spark.rb in your 
$PREFIX/shared/rcssserver3d dir.

regards,
Markus

[simspark-devel] Re: progress?

[Oliver O. <fr...@un...>]

Hi Hesham and Joschka,

On Thursday 01 December 2005 12:54, Hesham wrote:

> > Other than that, if there are any problems, please don't hesitate
> > to ask questions, maybe with CC to the other MC members, and also
> > Oli (fr...@un...) and Markus (rol...@un...).

yes, thanks for including us. I'd like to suggest to use the=20
simspark-devel mailing list for communication among developers.

> I have question about testing new codes. Do you have time to test the
> communication code? or I should send the code to all MC members and
> ask them to test it if they have time?

I would guess that Joschka selected skilled developers only :-), so=20
if the code compiles and works for you, please just check it in.=20
=46rom time to time, bugs are inevitable, but it is most convenient for=20
all developers to use the repository for distributing the code. We can=20
test it from there.

> Thank you so much, I know managing an open source project is not easy
> (IMHO it's extremely hard). I hope I can help at least by being on
> time :-)

Thanks for your work. I'll probably be back helping to implement=20
something in January.

cheers
Oliver

=2D-=20
Oliver Obst                     form follows function - Louis Sullivan
AI Research Group              =20
U Koblenz    B219                   https://www.uni-koblenz.de/~fruit/