-
Notifications
You must be signed in to change notification settings - Fork 0
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
Creates the backend of TopicInterfacePlugin. #385
Creates the backend of TopicInterfacePlugin. #385
Conversation
agalbachicar
commented
Apr 20, 2021
- Subscribes to a topic and reads a google::protobuf::message,
- parses the message into MessageWidget class to access the information and types easier.
- Prints data to ignerr to show it works -- will change once the UI is in place.
- Subscribes to a topic and reads a google::protobuf::message, - parses the message into MessageWidget class to access the information and types easier. - Prints data to ignerr to show it works -- will change once the UI is in place.
Note for the reviewers: there are several TODOs. I wanted to split the parsing procedure from the UI procedure because it is a bunch of code so far. Here is a dump of the scene with delphyne_mali2:
|
In 19098ce I fixed the serialization string. I tested it with a shorter message, /clock:
And the igntion message defition is:
where Time is:
and Header:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice Job! LGTM
I will take a look again at the reflection logic later on just to be sure, I didn't fully get when fieldDescriptor->is_repeated()
.
|
||
const auto fieldType = fieldDescriptor->type(); | ||
|
||
const std::string fieldName = fieldDescriptor->name(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't directly call it scopedName
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Thanks!
private: | ||
/// @brief Parses @p msg and stores the values into `variantValue` or | ||
/// `children`. | ||
void Parse(google::protobuf::Message* msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: msg
to _msg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
const std::unordered_map<std::string, std::unique_ptr<MessageWidget>>& Children() const { return children; } | ||
|
||
private: | ||
/// @brief Parses @p msg and stores the values into `variantValue` or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Parse
method storing the values in variantValue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
// Build the message widget. | ||
if (!msgType.empty()) { | ||
auto newMsg = ignition::msgs::Factory::New(msgType, ""); | ||
if (!newMsg) { | ||
ignerr << "Unable to create message of type[" << msgType << "] " | ||
<< "widget will be initialized when a message is received." << std::endl; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't get what this is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. Nothing, it was part of the migration but it is not needed.
|
||
signals: | ||
|
||
protected: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove not used sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM