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

4 ➡️ 5 #246

Merged
merged 19 commits into from
Feb 17, 2021
Merged

4 ➡️ 5 #246

merged 19 commits into from
Feb 17, 2021

Conversation

chapulina
Copy link
Contributor

Port ign-rendering4 to main.

Branch comparison: main...ign-rendering4

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

ahcorde and others added 19 commits January 29, 2021 11:31
Signed-off-by: ahcorde <ahcorde@gmail.com>
* started incorporating heat signature to thermal objects

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

* promising results with hardcoded texture

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

* heat signature for one object is working

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

* enable multiple objects with different heat signatures

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

* allow for users to specify a custom temperature range

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

* add check back in to fix missing background

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

* Add temperature variations to thermal camera readings based on color (#211)

* vary non-heat source temp based on rgb balues

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* add more comments, var to set rgb to temp

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* add more doc

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* remove comment

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* feedback changes

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* added reference link to RGB/grayscale equation

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

Co-authored-by: Ashton Larkin <ashton@openrobotics.org>

* make sure user input temperature range is within [0,655.35] kelvin

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

* added heat signature thermal camera test

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

* added missing test texture and fixed codecheck

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

* revert addition of Ogre2Material::PbsToUnlitDatablock helper function

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

* address review feedback

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

* address windows warnings

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

Co-authored-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Guillaume Doisy <guillaume.doisy@wyca.fr>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
* add support for 8 bit thermal camera

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* doc

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* update test and changelog

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* style, feedback changes

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* add more doc

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
* Fix coding style issues (#225)

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* ⬆️  3.4.0 (#238)

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

* Update changelog and cmake

Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Ian Chen <ichen@osrfoundation.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
Co-authored-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina requested a review from adlarkin February 12, 2021 01:13
@chapulina chapulina requested a review from iche033 as a code owner February 12, 2021 01:13
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Feb 12, 2021
@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #246 (13db3f6) into main (c6fc79f) will increase coverage by 0.24%.
The diff coverage is 78.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
+ Coverage   57.01%   57.25%   +0.24%     
==========================================
  Files         151      151              
  Lines       15010    15181     +171     
==========================================
+ Hits         8558     8692     +134     
- Misses       6452     6489      +37     
Impacted Files Coverage Δ
include/ignition/rendering/ArrowVisual.hh 100.00% <ø> (ø)
include/ignition/rendering/AxisVisual.hh 100.00% <ø> (ø)
include/ignition/rendering/Camera.hh 100.00% <ø> (ø)
include/ignition/rendering/CompositeVisual.hh 100.00% <ø> (ø)
include/ignition/rendering/Geometry.hh 100.00% <ø> (ø)
include/ignition/rendering/Image.hh 100.00% <ø> (ø)
include/ignition/rendering/Light.hh 100.00% <ø> (ø)
include/ignition/rendering/Material.hh 100.00% <ø> (ø)
include/ignition/rendering/Mesh.hh 100.00% <ø> (ø)
include/ignition/rendering/Node.hh 100.00% <ø> (ø)
... and 12 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 c6fc79f...13db3f6. Read the comment docs.

@chapulina
Copy link
Contributor Author

This new test failure could be related to #189 or #235:

/Users/jenkins/workspace/ignition_rendering-ci-pr_any-homebrew-amd64/ign-rendering/test/integration/thermal_camera.cc:233
The difference between boxTemp and temp is 14, which exceeds boxTempRange, where
boxTemp evaluates to 310,
temp evaluates to 296, and
boxTempRange evaluates to 3.

@adlarkin
Copy link
Contributor

This new test failure could be related to #189 or #235

I took a look at homebrew CI. The failure is for the ThermalCameraBoxesUniformTemp test, which was actually already implemented before #189 and #235. That being said though, #189 and #235 did change some of the code slightly for processing uniform temperatures, so maybe these PRs are indeed related to this new failure. It's interesting that this failure only happened on MacOS and not Ubuntu. All checks passed on #235 as well, so I am not sure what's going on here.

@iche033 any ideas?

@chapulina
Copy link
Contributor Author

this failure only happened on MacOS and not Ubuntu.

I haven't looked closely into the code, but when this happens, I usually look for anything in the test that relies on timing, because the Homebrew CI machines are usually less powerful than the Ubuntu ones and may require more sleeping.

In any case, I retriggered the build to see if the test passes this time. I don't think we need to block this PR on a flaky test, but I think it would be good to address the flakiness back on ign-rendering4.

@adlarkin
Copy link
Contributor

I retriggered the build to see if the test passes this time

It looks like the test still failed on MacOS CI. I'm taking a closer look at this and will post an update here if I find anything that needs to be addressed. Let's hold off on merging this for now in case something needs to be fixed.

@iche033
Copy link
Contributor

iche033 commented Feb 12, 2021

hmm not sure why. The tests seem to be passing in ign-rendering4, e.g. in #241. Maybe we've changed something in edifice.

@adlarkin
Copy link
Contributor

adlarkin commented Feb 12, 2021

I tried to do some debugging about the new test failure. I opened up two draft PRs in order to trigger/compare CI (feel free to take a look at the comments I left in these PRs for more details):

I found that all tests pass on #248, but I am still seeing the MacOS failure on #247 even after trying a few workarounds. This makes me think that the new test failure is due to a combination of change(s) in this forward port and changes in edifice, since tests were passing on the dome and edifice branches without this forward port.

The interesting thing is that based on my testing in #247, it looks like only one pixel in the image fails the check (all of the other pixels pass), and I have no idea why (I'd expect that all pixels would either pass or fail, but not a mixture of each since the box should take up the whole image at this point in the test): #247 (comment)

I'm out of ideas at this point. Does anyone have any thoughts on what may be going on here?

@chapulina
Copy link
Contributor Author

I'll merge this to get some PRs moving forward while you peeps are looking into the thermal camera failure.

@chapulina chapulina merged commit 52c4ab0 into main Feb 17, 2021
@chapulina chapulina deleted the chapulina/4_to_5 branch February 17, 2021 02:57
@adlarkin
Copy link
Contributor

I'll merge this to get some PRs moving forward while you peeps are looking into the thermal camera failure.

I create an issue for this so that it can be tracked: #253

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants