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.

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

Fix ARE check on action type #741

Merged
merged 1 commit into from
Apr 8, 2021
Merged

Fix ARE check on action type #741

merged 1 commit into from
Apr 8, 2021

Conversation

pattacini
Copy link
Member

@pattacini pattacini commented Apr 8, 2021

This PR fixes #740.

I took the chance to analyze the problem by using the following snippet:

#include <cstdlib>
#include <typeinfo>
#include <string>
#include <iostream>

#include <iCub/action/actionPrimitives.h>

void test(const int i, const bool cond) {
    std::cout << "test " << i << ": "
              << (cond ? "passed ✅" : "failed ❌")
              << std::endl;
}

int main() {
    iCub::action::ActionPrimitives* action = new iCub::action::ActionPrimitivesLayer2();

    test(1, typeid(action).name() == typeid(iCub::action::ActionPrimitivesLayer2).name());
    test(2, typeid(*action).name() == typeid(iCub::action::ActionPrimitivesLayer2).name());
    test(3, dynamic_cast<iCub::action::ActionPrimitivesLayer2*>(action));
    test(4, typeid(action) == typeid(iCub::action::ActionPrimitivesLayer2));
    test(5, typeid(*action) == typeid(iCub::action::ActionPrimitivesLayer2));

    delete action;
    return EXIT_SUCCESS;
}

whose results are:

test 1: failed ❌
test 2: passed ✅
test 3: passed ✅
test 4: failed ❌
test 5: passed ✅

That said, I opted for the fix used in test no. 3 as it makes the ARE code a bit cleaner as well.

@Nicogene I found the use of std::is_same<,> along with decltype() a bit overkill plus I didn't manage to get it working at once.

@pattacini pattacini requested a review from Nicogene April 8, 2021 09:58
@pattacini pattacini self-assigned this Apr 8, 2021
Copy link
Member

@Nicogene Nicogene left a comment

Choose a reason for hiding this comment

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

Super thanks!

@pattacini pattacini merged commit 0bf92fe into master Apr 8, 2021
@pattacini pattacini deleted the fix/740 branch April 8, 2021 10:16
@pattacini
Copy link
Member Author

@Nicogene the final test would be to make it run on the real robot, also because it remains unclear how ARE was able to be working as expected in the past 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

actionsRenderingEngine: calib table stopped working
2 participants