Fix #5140 - Off by one error in measure_step_values when __SKIP__ is …

…true

* Move logic to Variant::isTruish to determine if a variant is trueish (whether that's bool true, str = "true", int != 0, double != 0.0
* Add  a OSRunner::prepareForMeasureRun() that takes no arg, protected, and friend OSWorkflow so we can use it without the penatly of instantiating an OSMeasure
* In Apply Measure, when __SKIP__, correctly call prepareForMEasureRun before incrementing the step. And do load the BCLMeasure to write the resultInfo like the workflow-gem was doing

src/measure/OSRunner.cpp

@@ -226,6 +226,10 @@ namespace measure {
   //}
 
   void OSRunner::prepareForMeasureRun(const OSMeasure& /*measure*/) {
+    prepareForMeasureRun();
+  }
+
+  void OSRunner::prepareForMeasureRun() {
     if (m_halted) {
       LOG(Error, "Wokflow halted, cannot prepate for measure run");
       return;

src/measure/OSRunner.hpp

@@ -19,6 +19,7 @@
 
 namespace openstudio {
 
+class OSWorkflow;
 class Workspace;
 class WorkspaceObject;
 
@@ -290,6 +291,13 @@ namespace measure {
     bool registerMsgAlsoLogs() const;
     void setRegisterMsgAlsoLogs(bool registerMsgAlsoLogs);
 
+   protected:
+    friend class openstudio::OSWorkflow;
+    // prepareForMeasureRun(OSMeasure) actually doesn't use the OSMeasure, but it's a check that an OSMeasure is there.
+    // This is the actual implementation though, and it's protected by OSWorkflow is friended so it can call it before SKIPPING a step without the
+    // performance penatly of actually loading and resolving the measure just to skip it
+    void prepareForMeasureRun();
+
    private:
     REGISTER_LOGGER("openstudio.measure.OSRunner");
 

src/utilities/data/Variant.cpp

@@ -25,41 +25,35 @@ VariantType Variant::variantType() const {
 }
 
 bool Variant::valueAsBoolean() const {
-  // Note JM 2019-05-17: This is functionally equivalent to `std::get<bool>(m_value)` except it doesn't risk throwing
-  // std::bad_variant_access which isn't available on mac prior to 10.14
-  if (const auto* p = std::get_if<bool>(&m_value)) {
-    return *p;
-  } else {
+  if (m_type != VariantType::Boolean) {
     LOG_AND_THROW("Variant does not hold a boolean");
   }
+  return std::get<bool>(m_value);
 }
 
 int Variant::valueAsInteger() const {
-  if (const auto* p = std::get_if<int>(&m_value)) {
-    return *p;
-  } else {
+  if (m_type != VariantType::Integer) {
     LOG_AND_THROW("Variant does not hold an int");
   }
+  return std::get<int>(m_value);
 }
 
 double Variant::valueAsDouble() const {
   if (m_type == VariantType::Integer) {
-    return (double)valueAsInteger();
+    return static_cast<double>(valueAsInteger());
   }
 
-  if (const auto* p = std::get_if<double>(&m_value)) {
-    return *p;
-  } else {
+  if (m_type != VariantType::Double) {
     LOG_AND_THROW("Variant does not hold a double");
   }
+  return std::get<double>(m_value);
 }
 
 std::string Variant::valueAsString() const {
-  if (const auto* p = std::get_if<std::string>(&m_value)) {
-    return *p;
-  } else {
+  if (m_type != VariantType::String) {
     LOG_AND_THROW("Variant does not hold a string");
   }
+  return std::get<std::string>(m_value);
 }
 
 // helper constant for the visitor below so we static assert we didn't miss a type
@@ -85,8 +79,27 @@ Json::Value Variant::valueAsJSON() const {
     m_value);
 }
 
+bool Variant::isTrueish() const {
+  return std::visit(
+    [](auto&& arg) -> bool {
+      using T = std::decay_t<decltype(arg)>;
+      if constexpr (std::is_same_v<T, bool>) {  // NOLINT(bugprone-branch-clone)
+        return arg;
+      } else if constexpr (std::is_same_v<T, int>) {
+        return arg != 0;
+      } else if constexpr (std::is_same_v<T, double>) {
+        return arg != 0.0;
+      } else if constexpr (std::is_same_v<T, std::string>) {
+        return arg == "true";
+      } else {
+        static_assert(always_false_v<T>, "non-exhaustive visitor!");
+      }
+    },
+    m_value);
+}
+
 std::ostream& operator<<(std::ostream& os, const Variant& variant) {
-  VariantType variantType = variant.variantType();
+  const VariantType variantType = variant.variantType();
   if (variantType == VariantType::String) {
     os << variant.valueAsString();
   } else if (variantType == VariantType::Double) {

src/utilities/data/Variant.hpp

@@ -73,6 +73,10 @@ class UTILITIES_API Variant
 
   Json::Value valueAsJSON() const;
 
+  // No matter the underlying type, will check if it's true-ish
+  // Used for skipping a measure. eg true, "true", 1, 1.0 are all trueish
+  bool isTrueish() const;
+
  private:
   REGISTER_LOGGER("openstudio.Variant");
 

src/workflow/ApplyMeasure.cpp

@@ -39,19 +39,8 @@ bool isStepMarkedSkip(const std::map<std::string, openstudio::Variant>& stepArgs
 
   // handle skip first
   if (!stepArgs.empty() && stepArgs.contains("__SKIP__")) {
-    // TODO: handling of SKIP is incomplete here, will need to increment the runner and co, and not process the measure
     const openstudio::Variant& argumentValue = stepArgs.at("__SKIP__");
-    VariantType variantType = argumentValue.variantType();
-
-    if (variantType == VariantType::String) {
-      skip_measure = openstudio::ascii_to_lower_copy(argumentValue.valueAsString()) == "true";
-    } else if (variantType == VariantType::Double) {
-      skip_measure = (argumentValue.valueAsDouble() != 0.0);
-    } else if (variantType == VariantType::Integer) {
-      skip_measure = (argumentValue.valueAsInteger() != 0);
-    } else if (variantType == VariantType::Boolean) {
-      skip_measure = argumentValue.valueAsBoolean();
-    }
+    skip_measure = argumentValue.isTrueish();
   }
   return skip_measure;
 }
@@ -76,20 +65,30 @@ void OSWorkflow::applyMeasures(MeasureType measureType, bool energyplus_output_r
 
     auto stepArgs = step.arguments();
     const bool skip_measure = isStepMarkedSkip(stepArgs);
-    if (skip_measure || runner.halted()) {  // TODO: or halted
+    if (skip_measure || runner.halted()) {
       if (!energyplus_output_requests) {
         if (runner.halted()) {
           LOG(Info, fmt::format("Skipping measure '{}' because simulation halted", measureDirName));
         } else {
           LOG(Info, fmt::format("Skipping measure '{}'", measureDirName));
+          // required to update current step
+          runner.prepareForMeasureRun();
+
           WorkflowStepResult result = runner.result();
           runner.incrementStep();
-          // addResultMeasureInfo(result, bclMeasure);  // TODO: Should I really instantiate the BCLMeasure just for this?
+          const auto measureDirPath_ = workflowJSON.findMeasure(measureDirName);
+          if (measureDirPath_) {
+            BCLMeasure bclMeasure(measureDirPath_.get());
+            workflow::util::addResultMeasureInfo(result, bclMeasure);
+            const std::string className = bclMeasure.className();
+            const auto measureName = step.name().value_or(className);
+            output_attributes[measureName]["applicable"] = openstudio::Variant(false);
+          } else {
+            LOG(Warn, "Could not find measure '" << measureDirName << "', but it's marked as skipped anyways so ignoring.");
+            output_attributes[step.name().value_or(measureDirName)]["applicable"] = openstudio::Variant(false);
+          }
           result.setStepResult(StepResult::Skip);
         }
-
-        // Technically here I would need to have gotten className from the measure to match workflow-gem, just to set applicable = false
-        output_attributes[step.name().value_or(measureDirName)]["applicable"] = openstudio::Variant(false);
       }
       continue;
     }