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

Improve documentation on how to replace Scene3D plugin #1698

Merged
merged 9 commits into from
Aug 21, 2023
Merged

Improve documentation on how to replace Scene3D plugin #1698

merged 9 commits into from
Aug 21, 2023

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Sep 12, 2022 •
edited
Loading

Following gazebosim/gz-gui#485:

  • Pointing to SDF code in Migration.md and reference it from the log message 781cecb
  • Fix format in 781cecb

Fixes gazebosim/gz-gui#484

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Sep 12, 2022
This was referenced Sep 12, 2022
Closed
Open
@j-rivero
Fix format
c192f21 
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Fix code
80f9be8
ahcorde
ahcorde requested changes Sep 13, 2022
View reviewed changes
Migration.md
Comment on lines +117 to +118
SDF code for all these can be found in:
https://github.com/gazebosim/gz-sim/blob/ff1c82b41e548dfdc8076374f9500db2df2c35a1/examples/worlds/minimal_scene.sdf#L29-L128
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
SDF code for all these can be found in:
https://github.com/gazebosim/gz-sim/blob/ff1c82b41e548dfdc8076374f9500db2df2c35a1/examples/worlds/minimal_scene.sdf#L29-L128
SDF code for all these can be found in:
https://github.com/gazebosim/gz-sim/blob/gz-sim7/examples/worlds/minimal_scene.sdf#L29-L128

Copy link
Contributor Author

@j-rivero j-rivero Sep 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I was unsure about making this change: it would be great since it will get possible bug fixes/udpates on gz-sim7 branch but also could include potential non-wanted changes done to that file or reordering breaking the selection of lines. @jennuine what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I think it's better to point to a specific commit so that it ensures the ordering is the same

jennuine
jennuine reviewed Sep 13, 2022
View reviewed changes
Migration.md
Comment on lines +117 to +118
SDF code for all these can be found in:
https://github.com/gazebosim/gz-sim/blob/ff1c82b41e548dfdc8076374f9500db2df2c35a1/examples/worlds/minimal_scene.sdf#L29-L128
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I think it's better to point to a specific commit so that it ensures the ordering is the same

src/gui/Gui.cc Outdated
Comment on lines 460 to 461
msg += " SDF code to replace GzScene3D is available at " +
" https://github.com/gazebosim/gz-sim/blob/gz-sim7/Migration.md\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add this to the the string above, where msg is being constructed (lines 440-442). Also, instead of printing a link, I would just say "see migration guide for more details" but this is only my opinion.

For example, lines 440-442:

            std::string msg{
                "The [GzScene3D] GUI plugin has been removed since Garden "
                "(see migration guide for more details). "
                "Loading the following plugins instead:\n"};

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 moved the message to that lines in 8325cf5. I would prefer to facilitate people the link so migration can work just by clicking, we (developers) tend to be lazy. Not strong on this if you really prefer to avoid URLs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not strong on this either so sounds good to me

jennuine
jennuine reviewed Sep 14, 2022
View reviewed changes
src/gui/Gui.cc Outdated
Comment on lines 460 to 461
msg += " SDF code to replace GzScene3D is available at " +
" https://github.com/gazebosim/gz-sim/blob/gz-sim7/Migration.md\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not strong on this either so sounds good to me

jennuine
jennuine approved these changes Sep 14, 2022
View reviewed changes
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, left one minor comment

@j-rivero @jennuine
Update Migration.md
981d8f5 
Co-authored-by: Jenn Nguyen <[email protected]>
@j-rivero j-rivero requested a review from ahcorde September 14, 2022 18:56
@j-rivero
Fix length of lines
bb0f8b4 
Signed-off-by: Jose Luis Rivero <[email protected]>
@codecov
Copy link

codecov bot commented Sep 14, 2022 •
edited
Loading

Codecov Report

Merging #1698 (bb0f8b4) into gz-sim7 (3432dcb) will decrease coverage by 1.17%.
The diff coverage is 0.00%.

❗ Current head bb0f8b4 differs from pull request most recent head 3990bbb. Consider uploading reports for the commit 3990bbb to get more accurate results

@@             Coverage Diff             @@
##           gz-sim7    #1698      +/-   ##
===========================================
- Coverage    65.07%   63.90%   -1.17%     
===========================================
  Files          354      334      -20     
  Lines        28732    26344    -2388     
===========================================
- Hits         18697    16836    -1861     
+ Misses       10035     9508     -527
Files Changed Coverage Δ
src/gui/Gui.cc 68.22% <0.00%> (ø)

... and 86 files with indirect coverage changes

@azeey
Copy link
Contributor

azeey commented Feb 8, 2023

Can you resolve the conflicts @j-rivero? Otherwise, this looks like it's good to go. And @ahcorde for one more look.

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@azeey
Copy link
Contributor

azeey commented Aug 21, 2023

@ahcorde can you give this another look?

@ahcorde ahcorde requested a review from mjcarroll as a code owner August 21, 2023 22:43
@ahcorde ahcorde enabled auto-merge (squash) August 21, 2023 22:44
@ahcorde ahcorde merged commit 1e2a832 into gz-sim7 Aug 21, 2023
@ahcorde ahcorde deleted the jrivero/add_sdf_code_for_scene3D branch August 21, 2023 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahcorde ahcorde ahcorde approved these changes

@jennuine jennuine jennuine approved these changes

@mjcarroll mjcarroll Awaiting requested review from mjcarroll mjcarroll is a code owner

Assignees
No one assigned
Labels
beta Targeting beta release of upcoming collection 🌱 garden Ignition Garden
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use Migration.md instructions for replacing GzScene3D in Garden lead to a crash
4 participants
@j-rivero @azeey @ahcorde @jennuine