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

Grid config: set values from startup and improve layout #324

Merged
merged 12 commits into from Dec 8, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Dec 2, 2021

🎉 New feature

Summary

  • Make widgets expand to fill the entire plugin
  • Increase maximum values
  • Make use of sceneFromFirstRenderEngine to reduce duplication
  • Choose number of decimals according to widget width
  • Make it possible to set initial values from XML
  • Add example

Note to self: backport to ign-gazebo3

Test it

ign gui -v 4 -c ign-gui/examples/config/grid_config.config

grid_config

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Dec 2, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Dec 2, 2021
@chapulina chapulina moved this from Inbox to In review in Core development Dec 2, 2021
ahcorde
ahcorde previously requested changes Dec 3, 2021
src/plugins/grid_config/GridConfig.hh Outdated Show resolved Hide resolved
<cell_count>3</cell_count>
<vertical_cell_count>2</vertical_cell_count>
<cell_length>2</cell_length>
<pose>1 2 3 0 0 0</pose>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<pose>1 2 3 0 0 0</pose>
<pose>1 2 3 0 0 0</pose>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been trying to get in the habit of separating position and rotation with an extra space as suggested on Specifying pose: Proposal for a better pose:

libsdformat and SDFormat tutorials should encourage additional whitespace, e.g. to separate translation and rotation, use 3 spaces (instead of 1) as a delimiter between values if they fit on one line, or use a newline (possibly with hanging indents) if they do not fit on one line.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

LGTM, I left a minor comment & suggestion but nonblocking

examples/config/grid_config.config Show resolved Hide resolved
examples/config/grid_config.config Outdated Show resolved Hide resolved
@chapulina chapulina dismissed ahcorde’s stale review December 6, 2021 23:46

Comments addressed

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

I'm working on addressing @jennuine 's comments, and I'll also take the opportunity to consolidate GridConfig and Grid3d once and for all. This should close #183.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
…botics/ign-gui into chapulina/6/grid_config

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

I updated this PR to incorporate Grid3D's functionality, and removed the code for Grid3D. The functionality added mainly consists of managing multiple grids:

ign gui -v 4 -c examples/config/grid_config.config

grids

I also deleted the original Grid3D and now GridConfig is installed with both names.

@jennuine , I made quite a lot of changes from the original PR, mind taking another look? Thanks!

@chapulina chapulina linked an issue Dec 7, 2021 that may be closed by this pull request
@chapulina
Copy link
Contributor Author

While looking at the Grid3D code, I noticed a lot of it was still commented out because we never completed the migration from QWidget to QtQuick. Here's a gif with the original Grid3D. I miss having multiple views.

511624492-gridpr

So I'll just note here that the functionality that I migrated to GridConfig is just what was working on ign-gui6, not all the functionality from ign-gui0. Specifically, the addition and removal of grids from the plugin can come at a later time.

@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #324 (e144763) into ign-gui6 (3b57ac2) will decrease coverage by 1.49%.
The diff coverage is 25.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gui6     #324      +/-   ##
============================================
- Coverage     65.97%   64.48%   -1.50%     
============================================
  Files            34       36       +2     
  Lines          4782     4953     +171     
============================================
+ Hits           3155     3194      +39     
- Misses         1627     1759     +132     
Impacted Files Coverage Δ
src/plugins/grid_config/GridConfig.hh 100.00% <ø> (ø)
src/plugins/grid_config/GridConfig.cc 22.35% <25.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b57ac2...e144763. Read the comment docs.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina merged commit 7f047db into ign-gui6 Dec 8, 2021
Core development automation moved this from In review to Done Dec 8, 2021
@chapulina chapulina deleted the chapulina/6/grid_config branch December 8, 2021 03:57
@chapulina chapulina mentioned this pull request Dec 8, 2021
chapulina added a commit that referenced this pull request Dec 8, 2021
* Added log storing for ign-gui (#272)

Signed-off-by: Nikhil Nair <nikhilnicky972@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Don't crash if a plugin has invalid QML (#315)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Set marker point size from message (#317)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Don't set visual scale for point markers (#321)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Fix TopicEcho plugin message display (#322)

- Change binding of width property in delegate (see: https://stackoverflow.com/questions/63767669/parent-is-null-in-listview-delegate-after-upgrade-to-qt-5-15)
- Use scoped reference to model.display (see: https://forum.qt.io/topic/92085/using-qstringlistmodel-as-model-in-listview)

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* Use qmldir to define QML module with IgnSpinBox (#319)

Signed-off-by: William Wedler <william.wedler@resquared.com>
Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Add PreRender event to MinimalScene (#325)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Offer a way to disable warnings on marker manager (#326)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

* Fix codecheck (#329)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Fix codecheck (#332)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Grid config: set values from startup and improve layout (#324)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Nikhil Nair <43491351+NickNair@users.noreply.github.com>
Co-authored-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Co-authored-by: Will <1305536+zflat@users.noreply.github.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Jenn Nguyen <jenn@openrobotics.org>
@chapulina chapulina moved this from Done to Highlights in Core development Dec 10, 2021
@j-rivero j-rivero removed this from Highlights in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display grid - configure from launch file
3 participants