Pull request #780
closed[Planner] Add state space
Added by Jean-François Erdelyi over 8 years ago. Updated over 8 years ago.
Description
Replacement of Configuration by Robot_state.
Is an extension of the configurations space.
Pull from:
planners:
/home/jferdely/Public/pullreq/planners.git
branch: pullreq-state
studio:
/home/jferdely/Public/pullreq/studio.git
branch: pullreq-state
Files
| 0001-fix-RobotState-and-remove-useless-functions.patch (5.72 KB) 0001-fix-RobotState-and-remove-useless-functions.patch | fixes realted to comment #4 | Jules Waldhart, 2016-04-20 14:54 |
Updated by Jules Waldhart over 8 years ago
I rebased it upon current version of planner bf2db3f6 (with conflicts) and studio (no issue)
I put the resulting commits on:
git://redmine.laas.fr/laas/users/jwaldart/perso-move3d/planners.git branch pullreq-state.
--
It seems to work, the modification is quite hazardous, but seems coherent. I was afraid that there was a link btw some Configuration and double* in p3d, but I couldn't find any.
- The pull-request lacks code documentation esp. in cont_state.
- class names shall be in CamelCase.
- all should be writen in english (attribute _Etat in Robot_state)
- I'm not sure about the necessity to have a separated cont_state: won't be used somewhere else, and could even introduce confusion. To be discussed.
Good job otherwise!
But not yet a +1...
Updated by Jean-François Erdelyi over 8 years ago
New version "merge Cont_state and RobotState, rename robot_state to RobotState and refactor the project"
Pull from:
planners:
/home/jferdely/Public/pullreq/planners.git
branch: pullreq-state
studio:
/home/jferdely/Public/pullreq/studio.git
branch: pullreq-state
Updated by Jean-François Erdelyi over 8 years ago
EDIT : [erratum]
New version "merge Cont_state and RobotState, rename robot_state to RobotState and refactor the project"
Pull from:
planners:
/home/jferdely/Public/pullreq/planners.git
branch: pullreq
studio:
/home/jferdely/Public/pullreq/studio.git
branch: pullreq
Updated by Jules Waldhart over 8 years ago
- Status changed from New to In Progress
conditional +1
I detected a few inconsistencies that could be harmful in the future, in RobotState:- convertToRadian : needs to convert also the derivatives
- getConfigInDegree : idem
- operator= copies only the double* (configuration)
- operator==, != and equal() and: compares only the configuration.
- sub, add, mult idem (maybe unused -> delete them, or at least deprecate [1])
Also there is a bug:
There are cases where calling getConfigStruct causes segfault, probably because the state is not initialized. You need to test this in the method and return NULL.
--
Other future changes that could be needed: provide overloads to access to the nth derivative in each method that access to the configuration (double getActiveDof(uint), getEigenVector(), at(), operators () and [], ...
--
If the pull-request is urgent, can be accepted with just the bug correction, with issuing an high priority Bug including all this.
--
Also, a note for the future, think about putting re-factors in a separate commit, for readability (it's boring to have to sort real changes vs. re-factor changes when reading a commit)
--
footnotes
[1]: they are used only in HRICS_CF_TimeToCollision, which is dummy and can be modified (a reason why we need kinodyn motion planing :) )
Updated by Jules Waldhart over 8 years ago
- File 0001-fix-RobotState-and-remove-useless-functions.patch 0001-fix-RobotState-and-remove-useless-functions.patch added
I attach the file 0001-fix-RobotState-and-remove-useless-functions.patch which corrects some of the bugs presented in my previous comment.
Updated by Jean-François Erdelyi over 8 years ago
Modifications to remove the inconsistencies in RobotState
Pull from:
planners:
/home/jferdely/Public/pullreq/planners.git
branch: pullreq
Updated by Mamoun Gharbi over 8 years ago
planners:
First of all, please remove these warnings:
planners/src/HRI_co/home/magharbi/devel/move3d/pull-request/planners/src/API/ConfigSpace/RobotState.cpp: In member function ‘RobotState& RobotState::operator=(const RobotState&)’:
planners/src/API/ConfigSpace/RobotState.cpp:80:47: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
planners/src/API/ConfigSpace/RobotState.cpp: In member function ‘void RobotState::convertToRadian()’:
planners/src/API/ConfigSpace/RobotState.cpp:107:42: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
planners/src/API/ConfigSpace/RobotState.cpp: In member function ‘statePtr_t RobotState::getConfigInDegree()’:
planners/src/API/ConfigSpace/RobotState.cpp:136:42: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
planners/src/API/ConfigSpace/RobotState.cpp: In member function ‘void RobotState::setConfiguration(configPt)’:
planners/src/API/ConfigSpace/RobotState.cpp:171:58: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
planners/src/API/ConfigSpace/RobotState.cpp: In member function ‘bool RobotState::equal(RobotState&, bool)’:
planners/src/API/ConfigSpace/RobotState.cpp:483:39: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
planners/src/HRI_costspace/CostFunctions/HRICS_CF_TimeToCollision.cpp:74:1: warning: no return statement in function returning non-void [-Wreturn-type]
here are my comments:
- The git tree is not very clean: adding and removing file in the same pull request might not be the best idea (note: it is ok for this time but let's try to keep it as clean as possible)
- SetConfiguration in RobotState: it is not setting anymore the libmove3d struct, it might introduce errors in the currently used algorithms.
- dist() in RobotState: specify the kind of distance computed: might be needed later.
- Concerning all the commented functions in RobotState: it is maybe not used right now, but I think these functions should be restaured
- the commit configuration.cpp/hpp refactor rename with a capital at the begining.
studio:
same coment concerning the git tree.
Updated by Jean-François Erdelyi over 8 years ago
New version
Pull from:
planners:
/home/jferdely/Public/pullreq/planners.git
branch: pullreq
Updated by Jules Waldhart over 8 years ago
- Status changed from In Progress to Closed
Accepted and pulled into redmine
Updated by Jules Waldhart over 8 years ago
forgot to say I changed stuff wrt. the original pull-request (mainly to remove occurrences of Cont_state and Robot_state in the history):
in planners: squashed all the commits into one.
in studio: rewrite history, and moved some #includes from .h to .cpp (adding required forward declarations in .h)