Pull request #788: New ik management and multiple bugfixes - move3d - LAAS-CNRS Projects

Actions

Copy link

Pull request #788

closed

New ik management and multiple bugfixes

Added by Mamoun Gharbi over 8 years ago. Updated over 8 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Jules Waldhart

Spent time:

3:00 h

Description

- Introducing a new way to handle ik computations.
- Bugfixes in number of base functions (pick, place, navigateTo,...)

this pull-request concerns libmove3d, planners, studio, and the data:
/home/magharbi/pull-request/libmove3d
/home/magharbi/pull-request/planners
/home/magharbi/pull-request/studio
/home/magharbi/pull-request/data

all on branch master

Actions

Copy link

Updated by Mamoun Gharbi over 8 years ago

Tracker changed from Bug to Pull request

Actions

Copy link

Updated by Jules Waldhart over 8 years ago

Status changed from New to Rejected

Rejected because of lots of remarks involving major corrections and project has another pending pull-request.

core:

(not in this pullreq) why is pqp_robot_robot_list_collision_test activating collisions? (l.3940)
update comment to doc the new behaviour

planners:

(New IK interface): (see #785#note-3)
- enums in struct (ikStatusType, ikEnumTypes) (please read updated CodingRules), and use CapitalCamelCase, so: IkStatus::Value and IkType::Value;
- move (and rename) getIkTypeFromString, getIkTypeAsString in structs.
- for IkManager::getIkTypeAsObject() -> createIkObject();
- also, comments of #785#note-3 for factManager should be applied to propertyManager and IkManager.
- ParseJson* -> parseJson* in factsMgr and propMgr.
- (?) do not create IkManager::_instance in global scope, init it to NULL;
- AnaliticalIk::compute should be divided into lots of functions, esp. but not only the target_pos modification, the cntrt management; so that the switch is reduced to 5-10 lines per case.
([GTP] Using new Ik manager in GTP):
- isReachableByIKNoObs => isReachableByIKNoCol to stay consistent with move3d (p3d, pqp...)
- placeReachable::getRandomSol l.190 : unconditionnal g3d_draw... in loop.
(Bugfix: error when using geometricComputations): using infinity instead of P3D_HUGE would be so much more easier :) but no problem.
([GTP] Fixing small problems in mightabilities): IsSubset should accept const iterators or MUST take const references!

([GTP] Using DatabaseReader in GTP)
- useless #include "database/DatabaseReader.hpp" Properties/size.cpp
- doc about places where the data is retrieved from is false. Easiest way of documenting would be to give the last part of the path (eg. Facts/enabledProperties.json) and link to DatabaseReader doc.
- global_json_path and similar should be renamed in lots of places (json_path may be acceptable), mainly in entities I think
- commit message should also says it changes entities to use DBReader.

older:
- virtualFact: existance has no existence as an english word (it's a typo in at least 2 function names).
- what is PlanParam::GTPTypeCase, and why can't it be an attribute of a GTP class?
- what are mightabilitiesGrids: needs doc.

data:

remove change in logm3d.properties

Actions

Copy link

Updated by Jules Waldhart over 8 years ago

Note that it should be accepted after the comments are taken into account/discussed. But I preferred rejecting rather than conditionally +1.

Actions

Copy link

Updated by Mamoun Gharbi over 8 years ago

Status changed from Rejected to In Progress

up.

comments taken into account

Actions

Copy link

Updated by Jules Waldhart over 8 years ago

...but they are not there... ?

Actions

Copy link

Updated by Jules Waldhart over 8 years ago

Here is the list of remarks still true, those marked with X are estimated required for this pull-req to be accepted. Other have been discussed and are left for the record.
There are 5 mew remarks at the bottom.

core:

(not in this pullreq) why is pqp_robot_robot_list_collision_test activating collisions? (l.3940)
X update comment to doc the new behaviour

planners:

(New IK interface): (see #785#note-3)
- enums in struct (ikStatusType, ikEnumTypes) (please read updated CodingRules), and use CapitalCamelCase, so: IkStatus::Value and IkType::Value;
- move (and rename) getIkTypeFromString, getIkTypeAsString in structs.
- X also, comments of #785#note-3 for factManager should be applied to propertyManager and IkManager.
  - PropertyManager::getClassFromPropertyType => createObjectFromPropertyType
  - remove getPropertyTypeFromObject
- AnaliticalIk::compute should be divided into lots of functions, esp. but not only the target_pos modification, the cntrt management; so that the switch is reduced to 5-10 lines per case.
(Bugfix: error when using geometricComputations): using infinity instead of P3D_HUGE would be so much more easier :) but no problem.
([GTP] Fixing small problems in mightabilities): IsSubset should accept const iterators ~~or MUST take const references!~~ (done)

([GTP] Using DatabaseReader in GTP)
- X doc about places where the data is retrieved from is false. Easiest way of documenting would be to give the last part of the path (eg. Facts/enabledProperties.json) and link to DatabaseReader doc (not to the file!) .
older:
- what is PlanParam::GTPTypeCase, and why can't it be an attribute of a GTP class?

NEW REMARKS:

X mightabilitiesGrids: typo in doc: camputeMights -> computeMights() and document that method in order to have a link.
Actually, there is not enough doc in DatabaseReader, but there will be (#793)
X Do not create Fact- and PropertyTypeTranslation at global scope (init to NULL)
data
- X commited empty change in logm3d.properties
- X (?) commited pointless changes in gtpTest