Mutex Lock crash when initialising RadialLight when linking static library.

[gamepopper]

I've manged to build the demo from the source code without issues, however when trying to link the compiled lib into another project I get issues with the RadialLight class.

I define the RadialLight in the header of a game state class (using my own framework built on top of SFML) like so:

#pragma once

#include <VFrame/VState.h>
#include <Candle/Candle.hpp>

class PlayState : public VState
{
public:
	typedef VState VSUPERCLASS;
	PlayState() = default;

	virtual void Initialise() override;
	virtual void Update(float dt) override;
	virtual void Draw(sf::RenderTarget& RenderTarget) override;

	candle::RadialLight light;
	candle::EdgeVector edges;
};

Also, implement a test example in the cpp like so:

#include "PlayState.h"
#include <VFrame/VGlobal.h>

#include "PauseScreen.h"

#include <Candle/Candle.hpp>

void PlayState::Initialise()
{
	VSUPERCLASS::Initialise();

	light.setRange(150);
	edges.emplace_back(
		sf::Vector2f(200.f, 100.f),
		sf::Vector2f(200.f, 300.f));
}

void PlayState::Update(float dt)
{
	VSUPERCLASS::Update(dt);

	light.setPosition(VGlobal::p()->GetMousePosition());
	light.castLight(edges.begin(), edges.end());

	if (VGlobal::p()->Input->IsButtonPressed("Pause"))
	{
		OpenSubState(new PauseScreen());
	}
}

void PlayState::Draw(sf::RenderTarget& RenderTarget)
{
	VSUPERCLASS::Draw(RenderTarget);
	RenderTarget.draw(light);
}

It compiles in Visual Studio 2017, but when I try to run I get a crash from an Access Violation within MutexImpl::lock(). Going down the call stack, the source is the RadialLight class when calling the default constructor on the two sf::Texture objects within RadialLight.cpp:

namespace candle{
    const float BASE_RADIUS = 400.0f;
    bool l_texturesReady(false);
    sf::Texture l_lightTextureFade;
    sf::Texture l_lightTexturePlain;
      
    void initializeTextures(){

I've found one way to resolve the crash, however. You can change these two to pointers (or better, smart unique pointers) and initialise the pointers in the initializeTextures function, that way the textures don't try to initialise before the main function is called.

std::unique_ptr<sf::Texture> l_lightTextureFade;
std::unique_ptr<sf::Texture> l_lightTexturePlain;

If you want to avoid smart pointers for compatibility, you could use raw pointers but then you would need to add a way to delete the pointer when the user wants to close the application.

[gamepopper]

Thought I'd update, the pointer idea might not be as effective as I thought, since exiting the application results in a different crash.

[MiguelMJ]

Hi, gamepopper.
After doing some research I found that this is a problem with static/global SFML resources for years now.
sf::Texture cannot be static
sf::Texture crashes when used in a static object
I was not aware of this specific problem and. although I know the risks of the current design, I didn't want the user to initialize the textures themselves...
Using smart pointers could be a good idea (I think Candle already uses C++11 features, I'm not sure right now). Could you elaborate on what's the different crash you get with them?

[gamepopper]

I regret not writing down the exact function it crashed on, but I know it was another access violation, but instead, it took place from the sf::Texture destructor the moment I try to close the application.

In my attempt to reproduce it, I found it work fine if you set up a lighting area beforehand, it will construct and destruct the texture without issue, even when you close the application. However, if you don't set up a lighting area, you get an assertion error.

It's also in the sf::Texture destructor, unlike the static sf::Texture one I can't narrow this one down.

A raw-pointer approach I figured out is to include a static integer to count the number of RadialLight instances. Anytime the constructor is called, the counter increments, and whenever the destructor is called it decrements. If the count is zero when the destructor is called, the textures get deleted.

RadialLight::~RadialLight()
{
  instanceCount--;
  if (instanceCount == 0 &&
	  l_lightTextureFade &&
	  l_lightTexturePlain)
  {
	  delete l_lightTextureFade;
	  delete l_lightTexturePlain;
	  
	  l_lightTextureFade = nullptr;
	  l_lightTexturePlain = nullptr;
  }
}

At least with this approach, the textures won't be created until there is one RadialLight instance, and it will be destroyed when there are no more RadialLights.

[MiguelMJ]

The problem with this is that each time the counter reaches 0, the textures are required to be reinstantiated in case a new light is created. For that, after setting the pointers to nullptr, the instantiation flag should be also set to false:

l_texturesReady = false;

It is better than crashing, though.

[MiguelMJ]

Ok, I've replaced the textures l_lightTextureFade and l_lightTexturePlain with pointers to the render textures. I thought that another possible cause for the problem is that in the previous code, the textures were created in a sf::RenderTexture local to the initializeTexures function... So now both hazards are now removed.

I also implemented the counter of instances to remove the textures when they are not required. As you said, that would be a solution for raw-pointers, but I enabled that anyways because I'm not sure if a global std::unique_ptr<sf::RenderTexture> could be problematic at destruction time.
However, the errors we are talking about are not present in all compilers (I use GCC 9.3.0 and I've not had a problem like this) and this solution might cause some overhead when lights are being created and destroyed constantly and also. For this reason I didn't make its usage a default behavior. Instead I added the -DRADIAL_LIGHT_FIX compiler option to activate it.

I know this is not the best solution, and all this problems just are pointing to an underlying design problem... but for now, this may patch things up until a future version.

@all-contributors please add @gamepopper for bug report and testing

Let me know if you get it to work 😃