From 3a75dcf84db6ff132d5ffe24813d26b1018a0d97 Mon Sep 17 00:00:00 2001 From: Bradley Austin Davis Date: Thu, 7 Sep 2017 14:32:50 -0700 Subject: [PATCH 1/3] Fix crashes in entity rendering on OSX --- .../RenderableParticleEffectEntityItem.cpp | 93 ++++++++++--------- .../src/RenderableParticleEffectEntityItem.h | 3 +- .../src/RenderableTextEntityItem.cpp | 5 +- .../src/RenderableTextEntityItem.h | 5 +- .../entities/src/ParticleEffectEntityItem.cpp | 15 ++- .../entities/src/ParticleEffectEntityItem.h | 4 +- 6 files changed, 70 insertions(+), 55 deletions(-) diff --git a/libraries/entities-renderer/src/RenderableParticleEffectEntityItem.cpp b/libraries/entities-renderer/src/RenderableParticleEffectEntityItem.cpp index 7349e9147e..3328076911 100644 --- a/libraries/entities-renderer/src/RenderableParticleEffectEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableParticleEffectEntityItem.cpp @@ -91,20 +91,27 @@ void ParticleEffectEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePoi qCWarning(entitiesrenderer) << "Bad particle properties"; } } - if (_particleProperties != newParticleProperties) { + + if (resultWithReadLock([&]{ return _particleProperties != newParticleProperties; })) { _timeUntilNextEmit = 0; - _particleProperties = newParticleProperties; + withWriteLock([&]{ + _particleProperties = newParticleProperties; + }); } _emitting = entity->getIsEmitting(); - if (_particleProperties.textures.isEmpty()) { + bool hasTexture = resultWithReadLock([&]{ return _particleProperties.textures.isEmpty(); }); + if (hasTexture) { if (_networkTexture) { withWriteLock([&] { _networkTexture.reset(); }); } } else { - if (!_networkTexture || _networkTexture->getURL() != QUrl(_particleProperties.textures)) { + bool textureNeedsUpdate = resultWithReadLock([&]{ + return !_networkTexture || _networkTexture->getURL() != QUrl(_particleProperties.textures); + }); + if (textureNeedsUpdate) { withWriteLock([&] { _networkTexture = DependencyManager::get()->getTexture(_particleProperties.textures); }); @@ -115,15 +122,17 @@ void ParticleEffectEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePoi void ParticleEffectEntityRenderer::doRenderUpdateAsynchronousTyped(const TypedEntityPointer& entity) { // Fill in Uniforms structure ParticleUniforms particleUniforms; - particleUniforms.radius.start = _particleProperties.radius.range.start; - particleUniforms.radius.middle = _particleProperties.radius.gradient.target; - particleUniforms.radius.finish = _particleProperties.radius.range.finish; - particleUniforms.radius.spread = _particleProperties.radius.gradient.spread; - particleUniforms.color.start = _particleProperties.getColorStart(); - particleUniforms.color.middle = _particleProperties.getColorMiddle(); - particleUniforms.color.finish = _particleProperties.getColorFinish(); - particleUniforms.color.spread = _particleProperties.getColorSpread(); - particleUniforms.lifespan = _particleProperties.lifespan; + withReadLock([&]{ + particleUniforms.radius.start = _particleProperties.radius.range.start; + particleUniforms.radius.middle = _particleProperties.radius.gradient.target; + particleUniforms.radius.finish = _particleProperties.radius.range.finish; + particleUniforms.radius.spread = _particleProperties.radius.gradient.spread; + particleUniforms.color.start = _particleProperties.getColorStart(); + particleUniforms.color.middle = _particleProperties.getColorMiddle(); + particleUniforms.color.finish = _particleProperties.getColorFinish(); + particleUniforms.color.spread = _particleProperties.getColorSpread(); + particleUniforms.lifespan = _particleProperties.lifespan; + }); // Update particle uniforms memcpy(&_uniformBuffer.edit(), &particleUniforms, sizeof(ParticleUniforms)); } @@ -146,35 +155,26 @@ Item::Bound ParticleEffectEntityRenderer::getBound() { static const size_t VERTEX_PER_PARTICLE = 4; -bool ParticleEffectEntityRenderer::emitting() const { - return ( - _emitting && - _particleProperties.emission.rate > 0.0f && - _particleProperties.lifespan > 0.0f && - _particleProperties.polar.start <= _particleProperties.polar.finish - ); -} - -void ParticleEffectEntityRenderer::createParticle(uint64_t now) { +ParticleEffectEntityRenderer::CpuParticle ParticleEffectEntityRenderer::createParticle(uint64_t now, const Transform& baseTransform, const particle::Properties& particleProperties) { CpuParticle particle; - const auto& accelerationSpread = _particleProperties.emission.acceleration.spread; - const auto& azimuthStart = _particleProperties.azimuth.start; - const auto& azimuthFinish = _particleProperties.azimuth.finish; - const auto& emitDimensions = _particleProperties.emission.dimensions; - const auto& emitAcceleration = _particleProperties.emission.acceleration.target; - auto emitOrientation = _particleProperties.emission.orientation; - const auto& emitRadiusStart = glm::max(_particleProperties.radiusStart, EPSILON); // Avoid math complications at center - const auto& emitSpeed = _particleProperties.emission.speed.target; - const auto& speedSpread = _particleProperties.emission.speed.spread; - const auto& polarStart = _particleProperties.polar.start; - const auto& polarFinish = _particleProperties.polar.finish; + const auto& accelerationSpread = particleProperties.emission.acceleration.spread; + const auto& azimuthStart = particleProperties.azimuth.start; + const auto& azimuthFinish = particleProperties.azimuth.finish; + const auto& emitDimensions = particleProperties.emission.dimensions; + const auto& emitAcceleration = particleProperties.emission.acceleration.target; + auto emitOrientation = particleProperties.emission.orientation; + const auto& emitRadiusStart = glm::max(particleProperties.radiusStart, EPSILON); // Avoid math complications at center + const auto& emitSpeed = particleProperties.emission.speed.target; + const auto& speedSpread = particleProperties.emission.speed.spread; + const auto& polarStart = particleProperties.polar.start; + const auto& polarFinish = particleProperties.polar.finish; particle.seed = randFloatInRange(-1.0f, 1.0f); - particle.expiration = now + (uint64_t)(_particleProperties.lifespan * USECS_PER_SECOND); - if (_particleProperties.emission.shouldTrail) { - particle.position = _modelTransform.getTranslation(); - emitOrientation = _modelTransform.getRotation() * emitOrientation; + particle.expiration = now + (uint64_t)(particleProperties.lifespan * USECS_PER_SECOND); + if (particleProperties.emission.shouldTrail) { + particle.position = baseTransform.getTranslation(); + emitOrientation = baseTransform.getRotation() * emitOrientation; } // Position, velocity, and acceleration @@ -232,7 +232,7 @@ void ParticleEffectEntityRenderer::createParticle(uint64_t now) { particle.acceleration = emitAcceleration + randFloatInRange(-1.0f, 1.0f) * accelerationSpread; } - _cpuParticles.push_back(particle); + return particle; } void ParticleEffectEntityRenderer::stepSimulation() { @@ -244,14 +244,19 @@ void ParticleEffectEntityRenderer::stepSimulation() { const auto now = usecTimestampNow(); const auto interval = std::min(USECS_PER_SECOND / 60, now - _lastSimulated); _lastSimulated = now; + + particle::Properties particleProperties; + withReadLock([&]{ + particleProperties = _particleProperties; + }); - if (emitting()) { - uint64_t emitInterval = (uint64_t)(USECS_PER_SECOND / _particleProperties.emission.rate); - if (interval >= _timeUntilNextEmit) { + if (_emitting && particleProperties.emitting()) { + uint64_t emitInterval = particleProperties.emitIntervalUsecs(); + if (emitInterval > 0 && interval >= _timeUntilNextEmit) { auto timeRemaining = interval; while (timeRemaining > _timeUntilNextEmit) { // emit particle - createParticle(now); + _cpuParticles.push_back(createParticle(now, _modelTransform, particleProperties)); _timeUntilNextEmit = emitInterval; if (emitInterval < timeRemaining) { timeRemaining -= emitInterval; @@ -263,7 +268,7 @@ void ParticleEffectEntityRenderer::stepSimulation() { } // Kill any particles that have expired or are over the max size - while (_cpuParticles.size() > _particleProperties.maxParticles || (!_cpuParticles.empty() && _cpuParticles.front().expiration <= now)) { + while (_cpuParticles.size() > particleProperties.maxParticles || (!_cpuParticles.empty() && _cpuParticles.front().expiration <= now)) { _cpuParticles.pop_front(); } diff --git a/libraries/entities-renderer/src/RenderableParticleEffectEntityItem.h b/libraries/entities-renderer/src/RenderableParticleEffectEntityItem.h index e4c1d4be13..be2641c0c9 100644 --- a/libraries/entities-renderer/src/RenderableParticleEffectEntityItem.h +++ b/libraries/entities-renderer/src/RenderableParticleEffectEntityItem.h @@ -93,9 +93,8 @@ private: }; - void createParticle(uint64_t now); + static CpuParticle createParticle(uint64_t now, const Transform& baseTransform, const particle::Properties& particleProperties); void stepSimulation(); - bool emitting() const; particle::Properties _particleProperties; CpuParticles _cpuParticles; diff --git a/libraries/entities-renderer/src/RenderableTextEntityItem.cpp b/libraries/entities-renderer/src/RenderableTextEntityItem.cpp index d39139257b..8757bcbb0f 100644 --- a/libraries/entities-renderer/src/RenderableTextEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableTextEntityItem.cpp @@ -30,14 +30,11 @@ TextEntityRenderer::TextEntityRenderer(const EntityItemPointer& entity) : } - -void TextEntityRenderer::onRemoveFromSceneTyped(const TypedEntityPointer& entity) { +TextEntityRenderer::~TextEntityRenderer() { auto geometryCache = DependencyManager::get(); if (_geometryID && geometryCache) { geometryCache->releaseID(_geometryID); } - delete _textRenderer; - _textRenderer = nullptr; } bool TextEntityRenderer::needsRenderUpdateFromTypedEntity(const TypedEntityPointer& entity) const { diff --git a/libraries/entities-renderer/src/RenderableTextEntityItem.h b/libraries/entities-renderer/src/RenderableTextEntityItem.h index 1d8227069e..b0a72cf253 100644 --- a/libraries/entities-renderer/src/RenderableTextEntityItem.h +++ b/libraries/entities-renderer/src/RenderableTextEntityItem.h @@ -24,14 +24,13 @@ class TextEntityRenderer : public TypedEntityRenderer { using Pointer = std::shared_ptr; public: TextEntityRenderer(const EntityItemPointer& entity); - + ~TextEntityRenderer(); private: - virtual void onRemoveFromSceneTyped(const TypedEntityPointer& entity) override; virtual bool needsRenderUpdateFromTypedEntity(const TypedEntityPointer& entity) const override; virtual void doRenderUpdateAsynchronousTyped(const TypedEntityPointer& entity) override; virtual void doRender(RenderArgs* args) override; int _geometryID{ 0 }; - TextRenderer3D* _textRenderer; + std::shared_ptr _textRenderer; bool _faceCamera; glm::vec3 _dimensions; glm::vec3 _textColor; diff --git a/libraries/entities/src/ParticleEffectEntityItem.cpp b/libraries/entities/src/ParticleEffectEntityItem.cpp index bc6830e1a7..9bbf4323da 100644 --- a/libraries/entities/src/ParticleEffectEntityItem.cpp +++ b/libraries/entities/src/ParticleEffectEntityItem.cpp @@ -104,7 +104,7 @@ bool operator!=(const Properties& a, const Properties& b) { return !(a == b); } -bool particle::Properties::valid() const { +bool Properties::valid() const { if (glm::any(glm::isnan(emission.orientation))) { qCWarning(entities) << "Bad particle data"; return false; @@ -133,6 +133,19 @@ bool particle::Properties::valid() const { (radius.gradient.spread == glm::clamp(radius.gradient.spread, MINIMUM_PARTICLE_RADIUS, MAXIMUM_PARTICLE_RADIUS)); } +bool Properties::emitting() const { + return emission.rate > 0.0f && lifespan > 0.0f && polar.start <= polar.finish; + +} + +uint64_t Properties::emitIntervalUsecs() const { + if (emission.rate > 0.0f) { + return (uint64_t)(USECS_PER_SECOND / emission.rate); + } + return 0; +} + + EntityItemPointer ParticleEffectEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer entity { new ParticleEffectEntityItem(entityID) }; entity->setProperties(properties); diff --git a/libraries/entities/src/ParticleEffectEntityItem.h b/libraries/entities/src/ParticleEffectEntityItem.h index 6a3791f77e..9c0fd0ef95 100644 --- a/libraries/entities/src/ParticleEffectEntityItem.h +++ b/libraries/entities/src/ParticleEffectEntityItem.h @@ -160,7 +160,9 @@ namespace particle { Properties() {}; Properties(const Properties& other) { *this = other; } bool valid() const; - + bool emitting() const; + uint64_t emitIntervalUsecs() const; + Properties& operator =(const Properties& other) { color = other.color; alpha = other.alpha; From 0f465da92f2c684bcd9836118ca3e679c3ade064 Mon Sep 17 00:00:00 2001 From: Bradley Austin Davis Date: Thu, 7 Sep 2017 14:45:35 -0700 Subject: [PATCH 2/3] Various Mac fixes --- interface/src/main.cpp | 23 ++++++++++++++----- .../src/EntityTreeRenderer.cpp | 7 +++++- libraries/gl/src/gl/GLHelpers.cpp | 7 ++++-- libraries/gpu-gl/src/gpu/gl/GLTexture.cpp | 12 ++++++---- libraries/ui/src/ui/OffscreenQmlSurface.cpp | 11 +++++---- 5 files changed, 41 insertions(+), 19 deletions(-) diff --git a/interface/src/main.cpp b/interface/src/main.cpp index 503daa177d..cb90160cfe 100644 --- a/interface/src/main.cpp +++ b/interface/src/main.cpp @@ -144,25 +144,33 @@ int main(int argc, const char* argv[]) { #endif } + + // FIXME this method of checking the OpenGL version screws up the `QOpenGLContext::globalShareContext()` value, which in turn + // leads to crashes when creating the real OpenGL instance. Disabling for now until we come up with a better way of checking + // the GL version on the system without resorting to creating a full Qt application +#if 0 // Check OpenGL version. // This is done separately from the main Application so that start-up and shut-down logic within the main Application is // not made more complicated than it already is. - bool override = false; + bool overrideGLCheck = false; + QJsonObject glData; { OpenGLVersionChecker openGLVersionChecker(argc, const_cast(argv)); bool valid = true; - glData = openGLVersionChecker.checkVersion(valid, override); + glData = openGLVersionChecker.checkVersion(valid, overrideGLCheck); if (!valid) { - if (override) { + if (overrideGLCheck) { auto glVersion = glData["version"].toString(); - qCDebug(interfaceapp, "Running on insufficient OpenGL version: %s.", glVersion.toStdString().c_str()); + qCWarning(interfaceapp, "Running on insufficient OpenGL version: %s.", glVersion.toStdString().c_str()); } else { - qCDebug(interfaceapp, "Early exit due to OpenGL version."); + qCWarning(interfaceapp, "Early exit due to OpenGL version."); return 0; } } } +#endif + // Debug option to demonstrate that the client's local time does not // need to be in sync with any other network node. This forces clock @@ -223,8 +231,9 @@ int main(int argc, const char* argv[]) { Application app(argcExtended, const_cast(argvExtended.data()), startupTime, runningMarkerExisted); +#if 0 // If we failed the OpenGLVersion check, log it. - if (override) { + if (overrideGLcheck) { auto accountManager = DependencyManager::get(); if (accountManager->isLoggedIn()) { UserActivityLogger::getInstance().insufficientGLVersion(glData); @@ -238,6 +247,8 @@ int main(int argc, const char* argv[]) { }); } } +#endif + // Setup local server QLocalServer server { &app }; diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 9f4a9a3c4a..d754d59721 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -91,7 +91,12 @@ void entitiesScriptEngineDeleter(ScriptEngine* engine) { }; // Wait for the scripting thread from the thread pool to avoid hanging the main thread - QThreadPool::globalInstance()->start(new WaitRunnable(engine)); + auto threadPool = QThreadPool::globalInstance(); + if (threadPool) { + threadPool->start(new WaitRunnable(engine)); + } else { + delete engine; + } } void EntityTreeRenderer::resetEntitiesScriptEngine() { diff --git a/libraries/gl/src/gl/GLHelpers.cpp b/libraries/gl/src/gl/GLHelpers.cpp index ab91ca0902..28982703dd 100644 --- a/libraries/gl/src/gl/GLHelpers.cpp +++ b/libraries/gl/src/gl/GLHelpers.cpp @@ -40,8 +40,11 @@ const QSurfaceFormat& getDefaultOpenGLSurfaceFormat() { int glVersionToInteger(QString glVersion) { QStringList versionParts = glVersion.split(QRegularExpression("[\\.\\s]")); - int majorNumber = versionParts[0].toInt(); - int minorNumber = versionParts[1].toInt(); + int majorNumber = 0, minorNumber = 0; + if (versionParts.size() >= 2) { + majorNumber = versionParts[0].toInt(); + minorNumber = versionParts[1].toInt(); + } return (majorNumber << 16) | minorNumber; } diff --git a/libraries/gpu-gl/src/gpu/gl/GLTexture.cpp b/libraries/gpu-gl/src/gpu/gl/GLTexture.cpp index 7758ddaf49..943b8148ef 100644 --- a/libraries/gpu-gl/src/gpu/gl/GLTexture.cpp +++ b/libraries/gpu-gl/src/gpu/gl/GLTexture.cpp @@ -467,12 +467,14 @@ void GLVariableAllocationSupport::updateMemoryPressure() { _demoteQueue = WorkQueue(); // Populate the existing textures into the queue - for (const auto& texture : strongTextures) { - // Race conditions can still leave nulls in the list, so we need to check - if (!texture) { - continue; + if (_memoryPressureState != MemoryPressureState::Idle) { + for (const auto& texture : strongTextures) { + // Race conditions can still leave nulls in the list, so we need to check + if (!texture) { + continue; + } + addToWorkQueue(texture); } - addToWorkQueue(texture); } } } diff --git a/libraries/ui/src/ui/OffscreenQmlSurface.cpp b/libraries/ui/src/ui/OffscreenQmlSurface.cpp index 84466f41b0..11790041db 100644 --- a/libraries/ui/src/ui/OffscreenQmlSurface.cpp +++ b/libraries/ui/src/ui/OffscreenQmlSurface.cpp @@ -528,6 +528,12 @@ void OffscreenQmlSurface::create() { connect(_quickWindow, &QQuickWindow::focusObjectChanged, this, &OffscreenQmlSurface::onFocusObjectChanged); + // acquireEngine interrogates the GL context, so we need to have the context current here + if (!_canvas->makeCurrent()) { + qFatal("Failed to make context current for QML Renderer"); + return; + } + // Create a QML engine. auto qmlEngine = acquireEngine(_quickWindow); @@ -540,11 +546,6 @@ void OffscreenQmlSurface::create() { // FIXME Compatibility mechanism for existing HTML and JS that uses eventBridgeWrapper // Find a way to flag older scripts using this mechanism and wanr that this is deprecated _qmlContext->setContextProperty("eventBridgeWrapper", new EventBridgeWrapper(this, _qmlContext)); - - if (!_canvas->makeCurrent()) { - qWarning("Failed to make context current for QML Renderer"); - return; - } _renderControl->initialize(_canvas->getContext()); // When Quick says there is a need to render, we will not render immediately. Instead, From 45a2c3d5d9d1b4a7a6762e36fa0e8f9724a995f2 Mon Sep 17 00:00:00 2001 From: Bradley Austin Davis Date: Fri, 8 Sep 2017 11:36:32 -0700 Subject: [PATCH 3/3] Modifying SDL initialization due to crash on OSX --- plugins/hifiSdl2/src/SDL2Manager.cpp | 98 +++++++++++++++------------- 1 file changed, 54 insertions(+), 44 deletions(-) diff --git a/plugins/hifiSdl2/src/SDL2Manager.cpp b/plugins/hifiSdl2/src/SDL2Manager.cpp index 021cb4dfec..a5376af24e 100644 --- a/plugins/hifiSdl2/src/SDL2Manager.cpp +++ b/plugins/hifiSdl2/src/SDL2Manager.cpp @@ -54,49 +54,6 @@ SDL_JoystickID SDL2Manager::getInstanceId(SDL_GameController* controller) { } void SDL2Manager::init() { - loadSettings(); - - auto preferences = DependencyManager::get(); - static const QString SDL2_PLUGIN { "Game Controller" }; - { - auto getter = [this]()->bool { return _isEnabled; }; - auto setter = [this](bool value) { - _isEnabled = value; - saveSettings(); - }; - auto preference = new CheckPreference(SDL2_PLUGIN, "Enabled", getter, setter); - preferences->addPreference(preference); - } - - bool initSuccess = (SDL_Init(SDL_INIT_GAMECONTROLLER | SDL_INIT_HAPTIC) == 0); - - if (initSuccess) { - int joystickCount = SDL_NumJoysticks(); - - for (int i = 0; i < joystickCount; i++) { - SDL_GameController* controller = SDL_GameControllerOpen(i); - - if (controller) { - SDL_JoystickID id = getInstanceId(controller); - if (!_openJoysticks.contains(id)) { - //Joystick* joystick = new Joystick(id, SDL_GameControllerName(controller), controller); - Joystick::Pointer joystick = std::make_shared(id, controller); - _openJoysticks[id] = joystick; - auto userInputMapper = DependencyManager::get(); - userInputMapper->registerDevice(joystick); - auto name = SDL_GameControllerName(controller); - _subdeviceNames << name; - emit joystickAdded(joystick.get()); - emit subdeviceConnected(getName(), name); - } - } - } - - _isInitialized = true; - } - else { - qDebug() << "Error initializing SDL2 Manager"; - } } QStringList SDL2Manager::getSubdeviceNames() { @@ -110,8 +67,61 @@ void SDL2Manager::deinit() { } bool SDL2Manager::activate() { + + // FIXME for some reason calling this code in the `init` function triggers a crash + // on OSX in PR builds, but not on my local debug build. Attempting a workaround by + // + static std::once_flag once; + std::call_once(once, [&]{ + loadSettings(); + + auto preferences = DependencyManager::get(); + static const QString SDL2_PLUGIN { "Game Controller" }; + { + auto getter = [this]()->bool { return _isEnabled; }; + auto setter = [this](bool value) { + _isEnabled = value; + saveSettings(); + }; + auto preference = new CheckPreference(SDL2_PLUGIN, "Enabled", getter, setter); + preferences->addPreference(preference); + } + + bool initSuccess = (SDL_Init(SDL_INIT_GAMECONTROLLER | SDL_INIT_HAPTIC) == 0); + + if (initSuccess) { + int joystickCount = SDL_NumJoysticks(); + + for (int i = 0; i < joystickCount; i++) { + SDL_GameController* controller = SDL_GameControllerOpen(i); + + if (controller) { + SDL_JoystickID id = getInstanceId(controller); + if (!_openJoysticks.contains(id)) { + //Joystick* joystick = new Joystick(id, SDL_GameControllerName(controller), controller); + Joystick::Pointer joystick = std::make_shared(id, controller); + _openJoysticks[id] = joystick; + auto userInputMapper = DependencyManager::get(); + userInputMapper->registerDevice(joystick); + auto name = SDL_GameControllerName(controller); + _subdeviceNames << name; + emit joystickAdded(joystick.get()); + emit subdeviceConnected(getName(), name); + } + } + } + + _isInitialized = true; + } else { + qDebug() << "Error initializing SDL2 Manager"; + } + }); + + if (!_isInitialized) { + return false; + } + InputPlugin::activate(); - auto userInputMapper = DependencyManager::get(); for (auto joystick : _openJoysticks) { userInputMapper->registerDevice(joystick);