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

Insert Fuel Models #263

Merged
merged 66 commits into from
Aug 14, 2020
Merged

Insert Fuel Models #263

merged 66 commits into from
Aug 14, 2020

Conversation

JShep1
Copy link

@JShep1 JShep1 commented Jul 27, 2020

Ready for Review.

A few things to consider

  • Should we add a timeout for the thread loading the fuel model info?
  • Should we cache the results from an owner-model query? This would reduce duplicate work Done
  • Codecheck is complaing about two unused struct variables stating that they are unused (isFuel and isDownloaded), but I make use of them so I'm not sure if there's another way to go about this or if we add something to make codecheck ignore those lines.

Demo:
fuelmodels

John Shepherd and others added 30 commits June 1, 2020 16:05
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@JShep1 JShep1 marked this pull request as ready for review July 28, 2020 02:39
John Shepherd added 3 commits July 27, 2020 19:52
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 requested a review from nkoenig July 28, 2020 03:53
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Did a preliminary review, I'll wait for #173 to be merged before reviewing again.

Should we add a timeout for the thread loading the fuel model info?

I'd say this is something that can be handled in a consistent way on ign-fuel-tools. I think we could set CURLOPT_TIMEOUT, which is defaulting to 0 (no timeout).

In any case, if there's something wrong with the server, it should return some error code pretty quickly. So I think this is not extremely important.

Codecheck is complaing about two unused struct variables

Yeah I think with our current cppcheck version all we can do is ignore those lines

src/gui/plugins/resource_spawner/ResourceSpawner.cc Outdated Show resolved Hide resolved
src/gui/plugins/resource_spawner/ResourceSpawner.qml Outdated Show resolved Hide resolved
src/gui/plugins/resource_spawner/ResourceSpawner.qrc Outdated Show resolved Hide resolved
src/gui/plugins/resource_spawner/ResourceSpawner.cc Outdated Show resolved Hide resolved
Base automatically changed from jshep1/local_models to ign-gazebo2 August 6, 2020 03:24
@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #263 into ign-gazebo2 will increase coverage by 24.38%.
The diff coverage is 76.71%.

Impacted file tree graph

@@               Coverage Diff                @@
##           ign-gazebo2     #263       +/-   ##
================================================
+ Coverage        53.78%   78.16%   +24.38%     
================================================
  Files              121      183       +62     
  Lines             5838     9917     +4079     
================================================
+ Hits              3140     7752     +4612     
+ Misses            2698     2165      -533     
Impacted Files Coverage Δ
include/ignition/gazebo/Server.hh 100.00% <ø> (ø)
...nclude/ignition/gazebo/components/Serialization.hh 61.29% <0.00%> (+61.29%) ⬆️
.../plugins/component_inspector/ComponentInspector.cc 9.41% <0.00%> (ø)
src/systems/breadcrumbs/Breadcrumbs.hh 100.00% <ø> (ø)
src/systems/diff_drive/DiffDrive.hh 100.00% <ø> (ø)
src/systems/joint_controller/JointController.hh 100.00% <ø> (ø)
...stems/joint_state_publisher/JointStatePublisher.hh 100.00% <ø> (ø)
src/systems/physics/Physics.cc 74.12% <ø> (+58.14%) ⬆️
src/systems/user_commands/UserCommands.hh 100.00% <ø> (ø)
src/systems/user_commands/UserCommands.cc 80.32% <16.66%> (ø)
... and 122 more

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 881aae7...c826d38. Read the comment docs.

John Shepherd added 2 commits August 6, 2020 16:27
Signed-off-by: John Shepherd <[email protected]>
chapulina added a commit that referenced this pull request Aug 11, 2020
Signed-off-by: Louise Poubel <[email protected]>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Works well! I just have the 2 trivial comments below, otherwise 🚢 it!

src/gui/plugins/resource_spawner/ResourceSpawner.cc Outdated Show resolved Hide resolved
@JShep1 JShep1 merged commit a974824 into ign-gazebo2 Aug 14, 2020
@JShep1 JShep1 deleted the jshep1/fuel_models branch August 14, 2020 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants