Actions
Pull request #788
closedNew 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:
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
Updated by Mamoun Gharbi over 8 years ago
- Tracker changed from Bug to Pull request
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
- (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.
- useless
- 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.
- remove change in logm3d.properties
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.
Updated by Mamoun Gharbi over 8 years ago
- Status changed from Rejected to In Progress
up.
comments taken into account
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.
- (not in this pullreq) why is pqp_robot_robot_list_collision_test activating collisions? (l.3940)
- X update comment to doc the new behaviour
- (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
Updated by Mamoun Gharbi over 8 years ago
- Status changed from In Progress to Closed
pushed on master
planners|60783a14
studio|fa35abbf
libmove3d|35423658
data|a6ba6acb
Actions