Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/29/complete semantic checks merged #57

Merged
merged 6 commits into from
Jul 1, 2015
Merged

Feature/29/complete semantic checks merged #57

merged 6 commits into from
Jul 1, 2015

Conversation

nunoguedelha
Copy link
Contributor

This change is a further step in the implementation of #29. (requirements 1,3,4 of requirements document which can be found here:
https://github.com/nunoguedelha/DevAnalysis/tree/feature/29/CompleteSemanticChecks/issue%2329_completeSemanticChecks)

Rotation and Position semantics fully working with errors redirected to Matlab even if not launching from terminal.
Transform semantics to be completed in next push.

@nunoguedelha
Initial refactoring and cleanup of Position and Rotation classes
2f03848 
Squashed commit of the following:

commit c997a70
Author: Nuno Guedelha <nuno.guedelha@iit.it>
Date:   Wed Jun 17 15:16:19 2015 +0200

    fixed and compiled Position and Rotation classes

commit e0cd94e
Author: Nuno Guedelha <nuno.guedelha@iit.it>
Date:   Tue Jun 16 13:14:26 2015 +0200

    initial cleanup, refactoring Position and Rotation classes
@nunoguedelha
Used reportErrorIf function in Position and Rotation functions
c64e19a
@nunoguedelha
Add cmake options for semantics checks ON/OFF. Adapt assert to bindings
66515ef
@nunoguedelha
regenerate after SWIG update nd fixes
655b888
@nunoguedelha
Regenerate iDynTree Matlab wrapper with cout and cerr redirected to M…
c56d893 
…atlab
traversaro
traversaro reviewed Jul 1, 2015
View reviewed changes
core-experimental/include/iDynTree/Core/Rotation.h
/**
* Copy constructor: create a Rotation from another RotationRaw and another RotationSemantics.
*/
Rotation(const RotationRaw & other, RotationSemantics & semantics);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was initially private I think, and I left it this way since I always protect attributes as much as possible. But here, we should use protected otherwise any derivate class won't have access to this attribute.
I can change it in my next push if it's ok for you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constructor is only for internal use in semantic operations of Rotation class (compose, inverse2,...)

@traversaro
Copy link
Member

General comments:

  • travis-ci/push is failing because it does not have the fix commited to master in 103b5e1 ( travis-ci/pr is fine anyway ).
  • What do you think if we drop namespace indentation in implementation of the class ? We lose 4 characters for each method, and we don't benefit in additional readability
  • I see that you add back a lot of trailing whitespace (i.e. whitespaces at the end of a line), that is not ideal
    because it can causes differences between file revisions to appear even if there is not real difference in the code. I use an option in my IDE to automatically remove trailing whitespace, I see there is a similar option in XCode : http://stackoverflow.com/questions/12643581/xcode-4-5-trailing-whitespace . It may be worth to enable it to simplify code reviewing and merging.

traversaro
traversaro reviewed Jul 1, 2015
View reviewed changes
core-experimental/src/RotationSemantics.cpp
@@ -54,47 +61,40 @@ namespace iDynTree

void RotationSemantics::setReferenceOrientationFrame(int _refOrientationFrame)
{
this->refOrientationFrame = _refOrientationFrame;
this->refOrientationFrame = this->coordinateFrame = _refOrientationFrame;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good way of enforcing the constraint on semantic, but we should mention it in the doxygen documentation, otherways this behaviour is really counterintuitive.

@traversaro
Copy link
Member

  • I likea the idea of avoiding using transform as a method name, but I am afraid that the name convertToNewCoordFrame will generated confusion with the CoordinateFrame used as a semantic attribute for some classes.. in the meanwhile we can keep this.

@traversaro
Copy link
Member

I revert the matlab generated files to the swig commit agreed in #53 , so we can merge. We can always fix the issue of cout/cerr redirection once the pr is merged.

@traversaro
Copy link
Member

I had some glitches but now all test pass, merging.

traversaro added a commit that referenced this pull request Jul 1, 2015
@traversaro
Merge pull request #57 from robotology/feature/29/CompleteSemanticChe…
7984559 
…cksMerged

Feature/29/complete semantic checks merged
@traversaro traversaro merged commit 7984559 into master Jul 1, 2015
@traversaro traversaro deleted the feature/29/CompleteSemanticChecksMerged branch July 1, 2015 09:29
@nunoguedelha
Copy link
Contributor Author

Thanks. I forked the swig repo and added my changes.
repo: https://github.com/nunoguedelha/swig.git
tag: LatestWorking --> the commit you recommended (before the memory refactor)
branch: addCoutRedirectionToMatlab

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nunoguedelha @traversaro