robotology / peripersonal-space

This repository deals with the implementation of peripersonal space representations on the iCub humanoid robot.
GNU General Public License v2.0
1 stars 4 forks source link

Tidy up branches #20

Closed pattacini closed 8 years ago

pattacini commented 8 years ago

We have to:

@alecive @matejhof

alecive commented 8 years ago

@pattacini we tagged the repo when we acquired data for the pps-paper. I think that, at least until published, we should keep them (plus, they don't hurt).

matejhof commented 8 years ago

Ok, after discussing with @alecive and the latest commit regarding demoAvoidance, I need to test the demoAvoidance on the robot and the functionality of the 2D representation. After that, we should be able to merge.

matejhof commented 8 years ago

demoAvoidance in feature/WYSIWYD works like a charm. make sure you take the updated PPS_with_Optical_Flow script on board when merging - new port name to connect to is:

<from>/visuoTactileRF/pps_events_aggreg:o</from>
 <to>/avoidance/data:i</to>
matejhof commented 8 years ago

1D learning works fine in feature/WYSIWYD

matejhof commented 8 years ago

2D learning seems fine two (it's hard to precisely see the effects of velocity because of the low res of velocity, 4 bins, and the noise on the velocity estimation). So we can merge feature/WYSIWYD into master I think!

pattacini commented 8 years ago

Cool! @alecive are you in charge of this merge?

alecive commented 8 years ago

Great! I will do that ASAP (that might not necessarily mean before the end of this week, though :) )

alecive commented 8 years ago

I am working on the merge. @matejhof two stylistic things I would like to point out:

  1. Try to avoid superlong lines but rather decompose them in multiple-line chunks (there was a line with 632 characters!). This would greatly improve readability of the code. Usually, people say 80 characters per line, but also 100 is ok :)
  2. Try to stick with the same convention I am using throughout the repo regarding brackets. I.e this is what I am using :

    for (whatever)
    {
       ....
    }

    This is what you have in the virtualContactGeneration:

    for (whatever){
       ....
    }

    Now, I don't really care about which one is better, since they are equivalent, and it all comes up to a matter of personal preferences and habits. What I care about is to at least be consistent with the other 95% of the code - which uses the first convention, because also this helps in code readability :smile:

Thanks!

pattacini commented 8 years ago

@alecive is definitely right and I do put forward his requests.

Please, @matejhof, do not underestimate these factors. Complying with these rules discriminates code that will die soon from code that will represent a robust building block :smile: (really!)

When extending/modifying others' code, you must adapt yourself to the conventions you find in it.

alecive commented 8 years ago

Ok I did it! It was long and painful and I couldn't test it but at least it compiles. Also, @matejhof other two things:

  1. some times I saw wrong indentations that were not consistent throughout the code. There were some 3-spaces indentations, and some 5-spaces indentations which I had to move back to 4-spaces. Please double check in the future that all your indentations are correct!
  2. It would be nice to add some thorough doxy-style documentation to the modules you created. Right now it is kind of difficult to navigate through their methods from an external user's perspective (as I am, sadly, right now :cry: )