-
Notifications
You must be signed in to change notification settings - Fork 44
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
Transport Topics Plugin #69
Conversation
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.
Looking good, I did an initial review 😉
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.
some quick style comments. I'll look more into code logic later.
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.
I noticed that there's always one item under the topic name with the message type, for example, Pose_V
below:
I think we could get rid of that extra level. And just have the field names:
/world/default/dynamic_pose/info
├── header
└── pose
Also, just a reminder to make the plottable fields draggable 😉
@chapulina I solved all style issues you mentioned |
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.
This is looking great! A couple more comments:
Right now there's no way to see a topic if it starts being published after the plugin was open. How about updating it periodically like this:
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.
works great in general, just have some comments from debugging the Scene
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.
Spotted missing topics during debugging recursive messages in Scene
Terminal output
[GUI] [Dbg] [TopicViewer.cc:197] msg name: Scene, msg field name: header, msg field type: Header
[GUI] [Dbg] [TopicViewer.cc:197] msg name: Scene, msg field name: ambient, msg field type: Color
[GUI] [Dbg] [TopicViewer.cc:197] msg name: Scene, msg field name: background, msg field type: Color
[GUI] [Dbg] [TopicViewer.cc:197] msg name: Scene, msg field name: sky, msg field type: Sky
[GUI] [Dbg] [TopicViewer.cc:197] msg name: Scene, msg field name: fog, msg field type: Fog
[GUI] [Dbg] [TopicViewer.cc:197] msg name: Scene, msg field name: model, msg field type: Model
[GUI] [Dbg] [TopicViewer.cc:197] msg name: Scene, msg field name: light, msg field type: Light
[GUI] [Dbg] [TopicViewer.cc:197] msg name: Scene, msg field name: joint, msg field type: Joint
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.
I did another pass. This is looking really neat! Did you give any thought to the periodic update?
#include <QStandardItem> | ||
#include <QStandardItemModel> | ||
#include <QHash> | ||
#include <QByteArray> |
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.
Did you get a chance to triage these, @AmrElsersy ?
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.
Left some comments on the test, it's looking pretty good, but I didn't try running it locally.
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.
Thanks for adding the test! I did another review. It looks good in general. I just had some nitpicks on style and minor questions.
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.
a couple lingering style issue, otherwise looks good to me! Also make sure to git pull
before working on local branch, I made some small commits along the way.
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.
Working for me, great work!
Besides the other comments, the only thing I'd like to ask is that you change all the indentation to be 2 spaces in the QML file. Right now there's a mix of 2 and 4 🙂 Thanks!
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! I just fixed a linter error on 3071d72.
Also, the new test is failing on CI. This should fix it on Ubuntu: gazebo-tooling/release-tools#255. And for Homebrew, we can disable it when #76 is merged forward.
3071d72
to
754e1e9
Compare
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!
…lds draggable Signed-off-by: AmrElsersy <[email protected]>
754e1e9
to
673c611
Compare
This appears to have caused a build regression on Windows: https://build.osrfoundation.org/job/ign_gui-ci-win/58/consoleFull#-20872881438ff58640-3599-4406-a210-216932f1748c FYI @AmrElsersy / @chapulina / @claireyywang |
…lds draggable (#69) Signed-off-by: AmrElsersy <[email protected]>
…lds draggable (#69) Signed-off-by: AmrElsersy <[email protected]>
Topic Viewer Plugin