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

Add ogre2 skybox support #168

Merged
merged 10 commits into from
Nov 5, 2020
Merged

Add ogre2 skybox support #168

merged 10 commits into from
Nov 5, 2020

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Oct 26, 2020 •
edited
Loading

Add new functions in Scene for setting background material (SetBackgroundMaterial) and also enabling skybox (SetSkyEnabled). The SetSkyEnabled function is just a convenient function to set the background material to the default skybox cubemap texture supplied by ign-rendering.

Currently only working in ogre2 with regular RGB Ogre2Camera. There has been some changes in Ogre2DepthCamera compositor setup in ign-rendering2 that has not merged into main yet so I'm planning to merge the changes forward first before updating Ogre2DepthCamera to support skybox, e.g. so that it's also visible in an ign-sensors' RgbdCameraSensor

see screenshot from ogre2_demo

ogre2_demo_sky

@codecov
Copy link

codecov bot commented Oct 26, 2020 •
edited
Loading

Codecov Report

Merging #168 into main will increase coverage by 0.42%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
+ Coverage   52.10%   52.52%   +0.42%     
==========================================
  Files         144      143       -1     
  Lines       13120    13274     +154     
==========================================
+ Hits         6836     6972     +136     
- Misses       6284     6302      +18
Impacted Files Coverage Δ
include/ignition/rendering/Scene.hh 0.00% <ø> (ø)
include/ignition/rendering/base/BaseScene.hh 0.00% <ø> (ø)
ogre2/src/Ogre2DepthCamera.cc 88.30% <29.16%> (ø)
src/base/BaseScene.cc 74.69% <45.45%> (ø)
ogre2/src/Ogre2Camera.cc 86.71% <60.00%> (ø)
ogre2/src/Ogre2Scene.cc 90.43% <90.47%> (ø)
ogre2/src/Ogre2RenderTarget.cc 80.24% <94.69%> (ø)
ogre2/src/Ogre2RenderEngine.cc 80.05% <100.00%> (ø)
/github/workspace/ogre2/src/Ogre2RenderPass.cc
...b/workspace/include/ignition/rendering/RayQuery.hh
... and 285 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 d18dd1c...adff3dd. Read the comment docs.

@chapulina chapulina self-requested a review October 26, 2020 19:05
chapulina
chapulina reviewed Oct 27, 2020
View reviewed changes
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 for me! I just have some smaller comments and questions.

ogre2/src/Ogre2DepthCamera.cc Outdated
if (validBackground)
{
Ogre::MaterialManager &matManager = Ogre::MaterialManager::getSingleton();
std::string skyMatName = "SkyBox_" + this->Name();
Copy link
Contributor

Choose a reason for hiding this comment

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

The SkyBox string is repeated in a few places. How about storing it somewhere as a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 0823885 and fbae460

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Does the SkyBox string in RenderTarget need to match the one in DepthCamera? Keeping both in the same place could help future readers understanding how everything fits together, and prevent errors in case one is changed but not the other.

I'll leave it up to you if you think it's worth it.

ogre2/src/Ogre2RenderTarget.cc Outdated Show resolved Hide resolved
ogre2/src/Ogre2Scene.cc Outdated Show resolved Hide resolved
ogre2/src/media/materials/programs/skybox_fs.glsl Show resolved Hide resolved
ogre2/src/media/materials/programs/skybox_vs.glsl Outdated Show resolved Hide resolved
test/integration/sky.cc Show resolved Hide resolved
include/ignition/rendering/Scene.hh Show resolved Hide resolved
chapulina
chapulina approved these changes Oct 29, 2020
View reviewed changes
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.

🌥️

@chapulina chapulina added the 🏢 edifice Ignition Edifice label Nov 4, 2020
@iche033 iche033 mentioned this pull request Nov 4, 2020
Merged
@chapulina
Copy link
Contributor

The ign-gazebo PR works well with this. I'll go ahead and merge this!

@chapulina chapulina merged commit e452c39 into main Nov 5, 2020
@chapulina chapulina deleted the ogre2_skybox branch November 5, 2020 00:03
@iche033 iche033 mentioned this pull request Dec 1, 2020
Closed
@chapulina chapulina mentioned this pull request Mar 25, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@iche033 @chapulina