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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deleting ogre2 when created (and deleted) in a thread #840

Merged
merged 2 commits into from
Apr 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ if ((NOT ${CMAKE_VERSION} VERSION_LESS 3.11) AND (NOT OpenGL_GL_PREFERENCE))
set(OpenGL_GL_PREFERENCE "GLVND")
endif()

gz_find_package(OpenGL
gz_find_package(OpenGL REQUIRED
COMPONENTS OpenGL
OPTIONAL_COMPONENTS EGL
REQUIRED_BY ogre ogre2
PKGCONFIG gl)

Expand Down
8 changes: 8 additions & 0 deletions ogre2/src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ target_link_libraries(${ogre2_target}
terra
GzOGRE2::GzOGRE2)

if (TARGET OpenGL::EGL)
target_link_libraries(${ogre2_target}
PRIVATE
OpenGL::EGL
)
add_definitions(-DHAVE_EGL=1)
endif()

set (versioned ${CMAKE_SHARED_LIBRARY_PREFIX}${PROJECT_NAME_LOWER}-${engine_name}${CMAKE_SHARED_LIBRARY_SUFFIX})
set (unversioned ${CMAKE_SHARED_LIBRARY_PREFIX}${PROJECT_NAME_NO_VERSION_LOWER}-${engine_name}${CMAKE_SHARED_LIBRARY_SUFFIX})

Expand Down
35 changes: 27 additions & 8 deletions ogre2/src/Ogre2RenderEngine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@
# include <GL/glxext.h>
#endif

#if HAVE_EGL
#include <EGL/egl.h>
#endif

class GZ_RENDERING_OGRE2_HIDDEN
gz::rendering::Ogre2RenderEnginePrivate
{
Expand Down Expand Up @@ -146,6 +150,12 @@

this->dataPtr->hlmsPbsTerraShadows.reset();

if (this->window)
{
this->window->destroy();
this->window = nullptr;
}

if (this->ogreRoot)
{
// Clean up any textures that may still be in flight.
Expand All @@ -166,6 +176,7 @@
}
catch (...)
{
gzerr << "Error deleting ogre root " << std::endl;
}
this->ogreRoot = nullptr;
}
Expand Down Expand Up @@ -193,6 +204,12 @@
}
}
#endif

#if HAVE_EGL
// release egl per-thread state otherwise this causes a crash on exit if
// ogre is created and deleted in a thread
eglReleaseThread();
#endif
}

//////////////////////////////////////////////////
Expand Down Expand Up @@ -391,6 +408,7 @@
{
this->CreateContext();
}

this->CreateRoot();
this->CreateOverlay();
this->LoadPlugins();
Expand Down Expand Up @@ -1043,7 +1061,7 @@
{
Ogre::StringVector paramsVector;
Ogre::NameValuePairList params;
window = nullptr;
this->window = nullptr;

if (this->dataPtr->graphicsAPI == GraphicsAPI::VULKAN)
{
Expand Down Expand Up @@ -1103,19 +1121,18 @@
#endif

int attempts = 0;
while (window == nullptr && (attempts++) < 10)
while (this->window == nullptr && (attempts++) < 10)
{
try
{
window = Ogre::Root::getSingleton().createRenderWindow(
this->window = Ogre::Root::getSingleton().createRenderWindow(
stream.str(), _width, _height, false, &params);
this->RegisterHlms();
}
catch(const std::exception &_e)
{
gzerr << " Unable to create the rendering window: " << _e.what()
<< std::endl;
window = nullptr;
this->window = nullptr;

Check warning on line 1135 in ogre2/src/Ogre2RenderEngine.cc

View check run for this annotation

Codecov / codecov/patch

ogre2/src/Ogre2RenderEngine.cc#L1135

Added line #L1135 was not covered by tests
}
}

Expand All @@ -1126,12 +1143,14 @@
return std::string();
}

if (window)
this->RegisterHlms();

if (this->window)
{
window->_setVisible(true);
this->window->_setVisible(true);

// Windows needs to reposition the render window to 0,0.
window->reposition(0, 0);
this->window->reposition(0, 0);
}
return stream.str();
}
Expand Down
5 changes: 3 additions & 2 deletions test/integration/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ set(tests
lidar_visual
render_pass
scene
segmentation_camera
shadows
segmentation_camera
shadows
sky
thermal_camera
load_unload
wide_angle_camera
)

Expand Down
58 changes: 58 additions & 0 deletions test/integration/load_unload.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright (C) 2023 Open Source Robotics Foundation
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

#include <gtest/gtest.h>

#include "CommonRenderingTest.hh"

class LoadUnloadTest : public testing::Test
{
/// \brief A thread that loads and unloads the render engine
public: void RenderThread()
{
gz::common::Console::SetVerbosity(4);

auto [envEngine, envBackend, envHeadless] = GetTestParams();

if (envEngine.empty())
{
GTEST_SKIP() << kEngineToTestEnv << " environment not set";
}

auto engineParams = GetEngineParams(envEngine, envBackend, envHeadless);
gz::rendering::RenderEngine *engine =
gz::rendering::engine(envEngine, engineParams);
if (!engine)
{
GTEST_SKIP() << "Engine '" << envEngine << "' could not be loaded"
<< std::endl;
}

gz::rendering::unloadEngine(envEngine);
}
};

/////////////////////////////////////////////////
TEST_F(LoadUnloadTest, Thread)
{
// verify that we can load and unload the render engine in a thread
std::thread renderThread = std::thread(&LoadUnloadTest::RenderThread, this);
EXPECT_TRUE(renderThread.joinable());
EXPECT_NO_THROW(renderThread.join());
}