From 8a33f86e463d588a91b455def21b07eaf304f13f Mon Sep 17 00:00:00 2001 From: James Turner Date: Sun, 25 Apr 2021 18:59:56 +0100 Subject: [PATCH] Error-reporting: improve UI/display behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Assess how critical the error is, and based on this, show a popup instead of the dialog in some cases. Add commands to allow delayed display of the main dialog, and stepping through multiple error reports in the dialog. Also add a new error category for shader errors, since these are always emitted from the render thread and we can’t easily attribute them to an aircraft, scenery or core feature. --- src/Main/ErrorReporter.cxx | 101 ++++++++++++++++++++++++++++++++----- 1 file changed, 89 insertions(+), 12 deletions(-) diff --git a/src/Main/ErrorReporter.cxx b/src/Main/ErrorReporter.cxx index 1b0678d78..c0864dcef 100644 --- a/src/Main/ErrorReporter.cxx +++ b/src/Main/ErrorReporter.cxx @@ -74,10 +74,13 @@ enum class Aggregation { MultiPlayer, Unknown, ///< error coudln't be attributed more specifcially OutOfMemory, ///< seperate category to give it a custom message - Traffic + Traffic, + ShadersEffects }; // these should correspond to simgear::ErrorCode enum +// they map to translateable strings in fgdata/Translations/sys.xml + string_list static_errorIds = { "error-missing-shader", "error-loading-texture", @@ -116,7 +119,8 @@ string_list static_categoryIds = { "error-category-multiplayer", "error-category-unknown", "error-category-out-of-memory", - "error-category-traffic"}; + "error-category-traffic", + "error-category-shaders"}; class RecentLogCallback : public simgear::LogCallback { @@ -221,6 +225,7 @@ public: bool haveShownToUser = false; OccurrenceVec errors; + bool isCritical = false; bool addOccurence(const ErrorOcurrence& err); }; @@ -241,7 +246,6 @@ public: void collectError(simgear::LoadFailure type, simgear::ErrorCode code, const std::string& details, const sg_location& location) { - SG_LOG(SG_GENERAL, SG_WARN, "Error:" << static_errorTypeIds.at(static_cast(type)) << " from " << static_errorIds.at(static_cast(code)) << "::" << details << "\n\t" << location.asString()); ErrorOcurrence occurrence{code, type, details, location, time(nullptr)}; // snapshot the top of the context stacks into our occurence data @@ -255,9 +259,24 @@ public: auto it = getAggregateForOccurence(occurrence); // add to the occurence, if it's not a duplicate - if (it->addOccurence(occurrence)) { - it->lastErrorTime.stamp(); - _reportsDirty = true; + if (!it->addOccurence(occurrence)) { + return; // duplicate, nothing else to do + } + + // log it once we know it's not a duplicate + SG_LOG(SG_GENERAL, SG_WARN, "Error:" << static_errorTypeIds.at(static_cast(type)) << " from " << static_errorIds.at(static_cast(code)) << "::" << details << "\n\t" << location.asString()); + + it->lastErrorTime.stamp(); + _reportsDirty = true; + + const auto ty = it->type; + // decide if it's a critical error or not + if ((ty == Aggregation::OutOfMemory) || (ty == Aggregation::MainAircraft) || (ty == Aggregation::InputDevice)) { + it->isCritical = true; + } + + if (code == simgear::ErrorCode::LoadEffectsShaders) { + // it->isCritical = true; } } @@ -330,8 +349,13 @@ public: }); assert(it != _aggregated.end()); _activeReportIndex = std::distance(_aggregated.begin(), it); + _displayNode->setBoolValue("have-next", _activeReportIndex < (_aggregated.size() - 1)); + } - flightgear::sentryReportUserError(static_categoryIds.at(catId), detailsTextStream.str()); + void sendReportToSentry(AggregateReport& report) + { + const int catId = static_cast(report.type); + flightgear::sentryReportUserError(static_categoryIds.at(catId), _displayNode->getStringValue()); } void writeReportToStream(const AggregateReport& report, std::ostream& os) const; @@ -341,6 +365,7 @@ public: bool dismissReportCommand(const SGPropertyNode* args, SGPropertyNode*); bool saveReportCommand(const SGPropertyNode* args, SGPropertyNode*); + bool showErrorReportCommand(const SGPropertyNode* args, SGPropertyNode*); }; auto ErrorReporter::ErrorReporterPrivate::getAggregateForOccurence(const ErrorReporter::ErrorReporterPrivate::ErrorOcurrence& oc) @@ -427,6 +452,13 @@ auto ErrorReporter::ErrorReporterPrivate::getAggregateForOccurence(const ErrorRe return getAggregate(Aggregation::FGData); } + // becuase we report shader errors from the main thread, they don't + // get attributed. Collect them into their own category, which also + // means we can display a more specific message + if (oc.code == simgear::ErrorCode::LoadEffectsShaders) { + return getAggregate(Aggregation::ShadersEffects); + } + return getAggregate(Aggregation::Unknown); } @@ -519,7 +551,6 @@ bool ErrorReporter::ErrorReporterPrivate::dismissReportCommand(const SGPropertyN // clear any values underneath displayNode? - _nextShowTimeout.stamp(); _reportsDirty = true; // set this so we check for another report to present _activeReportIndex = -1; @@ -527,6 +558,39 @@ bool ErrorReporter::ErrorReporterPrivate::dismissReportCommand(const SGPropertyN return true; } +bool ErrorReporter::ErrorReporterPrivate::showErrorReportCommand(const SGPropertyNode* args, SGPropertyNode*) +{ + std::lock_guard g(_lock); + + if (_aggregated.empty()) { + return false; + } + + if (args->getBoolValue("next")) { + _activeReportIndex++; + if (_activeReportIndex >= _aggregated.size()) { + return false; + } + } else if (args->hasChild("index")) { + _activeReportIndex = args->getIntValue("index"); + if ((_activeReportIndex < 0) || (_activeReportIndex >= _aggregated.size())) { + return false; + } + } else { + _activeReportIndex = 0; + } + + auto& report = _aggregated.at(_activeReportIndex); + presentErrorToUser(report); + + auto gui = globals->get_subsystem(); + if (!gui->getDialog("error-report")) { + gui->showDialog("error-report"); + } + + return true; +} + bool ErrorReporter::ErrorReporterPrivate::saveReportCommand(const SGPropertyNode* args, SGPropertyNode*) { if (_activeReportIndex < 0) { @@ -667,7 +731,8 @@ void ErrorReporter::preinit() void ErrorReporter::init() { - if (!d->_enabledNode) { + const auto developerMode = false; // fgGetBool("sim/developer-mode"); + if (developerMode || !d->_enabledNode) { SG_LOG(SG_GENERAL, SG_INFO, "Error reporting disabled"); simgear::setFailureCallback(simgear::FailureCallback()); simgear::setErrorContextCallback(simgear::ContextCallback()); @@ -677,6 +742,7 @@ void ErrorReporter::init() globals->get_commands()->addCommand("dismiss-error-report", d.get(), &ErrorReporterPrivate::dismissReportCommand); globals->get_commands()->addCommand("save-error-report-data", d.get(), &ErrorReporterPrivate::saveReportCommand); + globals->get_commands()->addCommand("show-error-report", d.get(), &ErrorReporterPrivate::showErrorReportCommand); // cache these values here d->_fgdataPathPrefix = globals->get_fg_root().utf8Str(); @@ -686,6 +752,7 @@ void ErrorReporter::init() void ErrorReporter::update(double dt) { bool showDialog = false; + bool showPopup = false; bool havePendingReports = false; // beginning of locked section @@ -731,7 +798,13 @@ void ErrorReporter::update(double dt) const auto ageSec = (n - report.lastErrorTime).toSecs(); if (ageSec > NoNewErrorsTimeout) { d->presentErrorToUser(report); - showDialog = true; + if (report.isCritical) { + showDialog = true; + } else { + showPopup = true; + } + + d->sendReportToSentry(report); // if we show one report, don't consider any others for now break; @@ -745,8 +818,9 @@ void ErrorReporter::update(double dt) } } // end of locked section - if (showDialog && flightgear::isHeadlessMode()) { + if (flightgear::isHeadlessMode()) { showDialog = false; + showPopup = false; } // do not call into another subsystem with our lock held, @@ -754,7 +828,6 @@ void ErrorReporter::update(double dt) if (showDialog) { auto gui = globals->get_subsystem(); gui->showDialog("error-report"); - // this needs a bit more thought, disabling for the now #if 0 // pause the sim when showing the popup @@ -762,6 +835,9 @@ void ErrorReporter::update(double dt) pauseArgs->setBoolValue("force-pause", true); globals->get_commands()->execute("do_pause", pauseArgs); #endif + } else if (showPopup) { + SGPropertyNode_ptr popupArgs(new SGPropertyNode); + globals->get_commands()->execute("show-error-notification-popup", popupArgs, nullptr); } } @@ -770,6 +846,7 @@ void ErrorReporter::shutdown() if (d->_enabledNode) { globals->get_commands()->removeCommand("dismiss-error-report"); globals->get_commands()->removeCommand("save-error-report-data"); + globals->get_commands()->removeCommand("show-error-report"); sglog().removeCallback(d->_logCallback.get()); } }