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

Handle non-positive object temperatures #243

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Feb 10, 2021

Related to gazebosim/gz-sim#621. This handles temperatures that are <= 0 kelvin for the thermal camea.

Signed-off-by: Ashton Larkin ashton@openrobotics.org

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example world 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

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #243 (3454c14) into ign-rendering4 (7d55d9b) will decrease coverage by 0.01%.
The diff coverage is 62.50%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           ign-rendering4     #243      +/-   ##
==================================================
- Coverage           53.50%   53.48%   -0.02%     
==================================================
  Files                 145      145              
  Lines               13753    13760       +7     
==================================================
+ Hits                 7358     7359       +1     
- Misses               6395     6401       +6     
Impacted Files Coverage Δ
ogre2/src/Ogre2ThermalCamera.cc 89.28% <62.50%> (-1.48%) ⬇️

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 7d55d9b...3454c14. Read the comment docs.

@chapulina chapulina moved this from Inbox to In progress in Core development Feb 10, 2021
@iche033
Copy link
Contributor

iche033 commented Feb 10, 2021

I made some tweaks in a3a8854 that clamps neg temperature to 0 and enable our shaders work with 0 temp value. This should make the test in gazebosim/gz-sim#621 pass

@adlarkin
Copy link
Contributor Author

I made some tweaks in a3a8854 that clamps neg temperature to 0

I just took a look and tested this out, it works for me! I'm marking this as ready for review. If we merge gazebosim/gz-sim#621, this will need to be merged and we will need to make a patch release of ign-rendering before making the related ign-gazebo release.

@adlarkin adlarkin marked this pull request as ready for review February 10, 2021 20:44
@adlarkin
Copy link
Contributor Author

As mentioned in gazebosim/gz-sim#621 (comment), I am holding off on merging this for now since it would effect ign-gazebo and ign-rendering releases that are currently in-process at the time of this writing. I will mark this as a draft and re-open it once the releases are complete.

@adlarkin adlarkin marked this pull request as draft February 10, 2021 20:55
// only accept positive temperature (in kelvin)
if (temp >= 0.0)
// if a non-positive temperature was given, clamp it to 0
if (foundTemp && temp < 0.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This only clamps the temp value when foundTemp is true. Is it possible for foundTemp to be false and temp to be < 0.0? If so, should you handle that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible for foundTemp to be false and temp to be < 0.0?

I don't think so. The only way foundTemp is set to true is if temperature is defined/attached to a visual, which would mean that <temperature> needs to be specified in the SDF file in the ign-gazebo context. If <temperature> is not specified, then this code should run (background object where object colors are used to define temperatures - since we're using rgb here, it shouldn't be possible to have an rgb value that is < 0): https://github.com/ignitionrobotics/ign-rendering/blob/8e0de0dd3e499c0fc0191a2cd8b794b0aab69980/ogre2/src/Ogre2ThermalCamera.cc#L387-L417

@adlarkin adlarkin marked this pull request as ready for review March 16, 2021 20:43
Core development automation moved this from In progress to In review Mar 16, 2021
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@adlarkin adlarkin force-pushed the adlarkin/thermal_temperature_checking branch from 2a40601 to 3454c14 Compare March 16, 2021 23:59
@adlarkin adlarkin merged commit 3454c14 into ign-rendering4 Mar 17, 2021
Core development automation moved this from In review to Done Mar 17, 2021
@adlarkin adlarkin deleted the adlarkin/thermal_temperature_checking branch March 17, 2021 00:31
@j-rivero j-rivero removed this from Done 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
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants