From 9f55f167b2eca69a30a6e9c3db0f627ce3553fc2 Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Fri, 15 Nov 2024 07:25:44 +0100 Subject: [PATCH 1/2] planner: fix calculations of variations In 8704a8b the code in cloneDiveplan() was replaced by a simple assignment statement. Alas, the original code was more complex: it copied only up to a certain point (it stopped at automatically generated steps). The new behavior made the calculations of variations fail, because a call to plan() adds deco stops. Therefore, copy the plan _before_ calling plan(). Fixes #4368 Signed-off-by: Berthold Stoeger --- qt-models/diveplannermodel.cpp | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/qt-models/diveplannermodel.cpp b/qt-models/diveplannermodel.cpp index 3fc4f0278..10ff60598 100644 --- a/qt-models/diveplannermodel.cpp +++ b/qt-models/diveplannermodel.cpp @@ -1118,17 +1118,20 @@ void DivePlannerPointsModel::updateDiveProfile() if (diveplan.is_empty()) return; + // For calculating variations, we need a copy of the plan. We have to copy _before_ + // calling plan(), because that adds deco stops. + bool computeVariations = isPlanner() && shouldComputeVariations(); + std::unique_ptr plan_copy; + if (computeVariations) + plan_copy = std::make_unique(diveplan); + deco_state_cache cache; struct deco_state plan_deco_state; plan(&plan_deco_state, diveplan, d, dcNr, decotimestep, cache, isPlanner(), false); updateMaxDepth(); - if (isPlanner() && shouldComputeVariations()) { - auto plan_copy = std::make_unique(); - lock_planner(); - *plan_copy = diveplan; - unlock_planner(); + if (computeVariations) { #ifdef VARIATIONS_IN_BACKGROUND // Since we're calling computeVariations asynchronously and plan_deco_state is allocated // on the stack, it must be copied and freed by the worker-thread. @@ -1252,6 +1255,7 @@ void DivePlannerPointsModel::computeVariations(std::unique_ptr auto deeper = plan(&ds, plan_copy, dive.get(), dcNr, 1, cache, true, false); save.restore(&ds, false); + plan_copy = *original_plan; second_to_last(plan_copy.dp).depth.mm -= delta_depth.mm; plan_copy.dp.back().depth.mm -= delta_depth.mm; if (my_instance != instanceCounter) @@ -1266,6 +1270,7 @@ void DivePlannerPointsModel::computeVariations(std::unique_ptr auto longer = plan(&ds, plan_copy, dive.get(), dcNr, 1, cache, true, false); save.restore(&ds, false); + plan_copy = *original_plan; plan_copy.dp.back().time -= delta_time.seconds; if (my_instance != instanceCounter) return; @@ -1308,15 +1313,16 @@ void DivePlannerPointsModel::createPlan(bool saveAsNew) removeDeco(); createTemporaryPlan(); + // For calculating variations, we need a copy of the plan. We have to copy _before_ + // calling plan(), because that adds deco stops. + std::unique_ptr plan_copy; + if (shouldComputeVariations()) + plan_copy = std::make_unique(diveplan); + plan(&ds_after_previous_dives, diveplan, d, dcNr, decotimestep, cache, isPlanner(), true); - if (shouldComputeVariations()) { - auto plan_copy = std::make_unique(); - lock_planner(); - *plan_copy = diveplan; - unlock_planner(); + if (shouldComputeVariations()) computeVariations(std::move(plan_copy), &ds_after_previous_dives); - } // Fixup planner notes. if (current_dive && d->id == current_dive->id) { From 4f7d5675712cc80e5481fa416cf7f8cb6aff2c85 Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Fri, 15 Nov 2024 14:07:36 +0100 Subject: [PATCH 2/2] planner: use value semantics for computeVariations() When computing the variations in a background thread, the code has to work on a copy of the dive plan and the deco state. Instead of passing a copy via a unique_ptr<>, simply use value semantics when calling computeVariations(). This does an unnecessary copy of the deco state, when computeVariations is not run in the background, but so what. Signed-off-by: Berthold Stoeger --- qt-models/diveplannermodel.cpp | 53 ++++++++++------------------------ qt-models/diveplannermodel.h | 3 +- 2 files changed, 16 insertions(+), 40 deletions(-) diff --git a/qt-models/diveplannermodel.cpp b/qt-models/diveplannermodel.cpp index 10ff60598..6824011b4 100644 --- a/qt-models/diveplannermodel.cpp +++ b/qt-models/diveplannermodel.cpp @@ -1121,9 +1121,9 @@ void DivePlannerPointsModel::updateDiveProfile() // For calculating variations, we need a copy of the plan. We have to copy _before_ // calling plan(), because that adds deco stops. bool computeVariations = isPlanner() && shouldComputeVariations(); - std::unique_ptr plan_copy; + struct diveplan plan_copy; if (computeVariations) - plan_copy = std::make_unique(diveplan); + plan_copy = diveplan; deco_state_cache cache; struct deco_state plan_deco_state; @@ -1133,26 +1133,10 @@ void DivePlannerPointsModel::updateDiveProfile() if (computeVariations) { #ifdef VARIATIONS_IN_BACKGROUND - // Since we're calling computeVariations asynchronously and plan_deco_state is allocated - // on the stack, it must be copied and freed by the worker-thread. - auto deco_copy = std::make_unique(plan_deco_state); - - // Ideally, we would pass the unique_ptrs to the lambda for QtConcurrent::run(). - // This, in principle, can be done as such: - // [ptr = std::move(ptr)] () mutable { f(std::move(ptr)) }; - // However, this make the lambda uncopyable and QtConcurrent::run() sadly - // uses copy semantics. - // So let's be pragmatic and do a release/reaquire pair. - // Somewhat disappointing, but what do you want to do? - // Note 1: this is now not exception safe, but Qt doesn't support - // exceptions anyway. - // Note 2: We also can't use the function / argument syntax of QtConcurrent::run(), - // because it likewise uses copy-semantics. How annoying. - QtConcurrent::run([this, plan = plan_copy.release(), deco = deco_copy.release()] () - { this->computeVariationsFreeDeco(std::unique_ptr(plan), - std::unique_ptr(deco)); }); + QtConcurrent::run([this, plan = std::move(plan_copy), deco = plan_deco_state] () + { this->computeVariations(std::move(plan), deco); }); #else - computeVariations(std::move(plan_copy), &plan_deco_state); + computeVariations(std::move(plan_copy), plan_deco_state); #endif final_deco_state = plan_deco_state; } @@ -1197,12 +1181,6 @@ int DivePlannerPointsModel::analyzeVariations(const std::vector &min, return (leftsum + rightsum) / 2; } -void DivePlannerPointsModel::computeVariationsFreeDeco(std::unique_ptr original_plan, std::unique_ptr previous_ds) -{ - computeVariations(std::move(original_plan), previous_ds.get()); - // Note: previous ds automatically free()d by virtue of being a unique_ptr. -} - // Return reference to second to last element. // Caller is responsible for checking that there are at least two elements. template @@ -1211,17 +1189,16 @@ auto &second_to_last(T &v) return *std::prev(std::prev(v.end())); } -void DivePlannerPointsModel::computeVariations(std::unique_ptr original_plan, const struct deco_state *previous_ds) +void DivePlannerPointsModel::computeVariations(struct diveplan original_plan, struct deco_state ds) { // nothing to do unless there's an original plan - if (!original_plan) + if (original_plan.dp.empty()) return; auto dive = std::make_unique(); copy_dive(d, dive.get()); deco_state_cache cache, save; struct diveplan plan_copy; - struct deco_state ds = *previous_ds; int my_instance = ++instanceCounter; save.cache(&ds); @@ -1239,7 +1216,7 @@ void DivePlannerPointsModel::computeVariations(std::unique_ptr depth_units = tr("ft"); } - plan_copy = *original_plan; + plan_copy = original_plan; if (plan_copy.dp.size() < 2) return; if (my_instance != instanceCounter) @@ -1247,7 +1224,7 @@ void DivePlannerPointsModel::computeVariations(std::unique_ptr auto original = plan(&ds, plan_copy, dive.get(), dcNr, 1, cache, true, false); save.restore(&ds, false); - plan_copy = *original_plan; + plan_copy = original_plan; second_to_last(plan_copy.dp).depth.mm += delta_depth.mm; plan_copy.dp.back().depth.mm += delta_depth.mm; if (my_instance != instanceCounter) @@ -1255,7 +1232,7 @@ void DivePlannerPointsModel::computeVariations(std::unique_ptr auto deeper = plan(&ds, plan_copy, dive.get(), dcNr, 1, cache, true, false); save.restore(&ds, false); - plan_copy = *original_plan; + plan_copy = original_plan; second_to_last(plan_copy.dp).depth.mm -= delta_depth.mm; plan_copy.dp.back().depth.mm -= delta_depth.mm; if (my_instance != instanceCounter) @@ -1263,14 +1240,14 @@ void DivePlannerPointsModel::computeVariations(std::unique_ptr auto shallower = plan(&ds, plan_copy, dive.get(), dcNr, 1, cache, true, false); save.restore(&ds, false); - plan_copy = *original_plan; + plan_copy = original_plan; plan_copy.dp.back().time += delta_time.seconds; if (my_instance != instanceCounter) return; auto longer = plan(&ds, plan_copy, dive.get(), dcNr, 1, cache, true, false); save.restore(&ds, false); - plan_copy = *original_plan; + plan_copy = original_plan; plan_copy.dp.back().time -= delta_time.seconds; if (my_instance != instanceCounter) return; @@ -1315,14 +1292,14 @@ void DivePlannerPointsModel::createPlan(bool saveAsNew) // For calculating variations, we need a copy of the plan. We have to copy _before_ // calling plan(), because that adds deco stops. - std::unique_ptr plan_copy; + struct diveplan plan_copy; if (shouldComputeVariations()) - plan_copy = std::make_unique(diveplan); + plan_copy = diveplan; plan(&ds_after_previous_dives, diveplan, d, dcNr, decotimestep, cache, isPlanner(), true); if (shouldComputeVariations()) - computeVariations(std::move(plan_copy), &ds_after_previous_dives); + computeVariations(std::move(plan_copy), ds_after_previous_dives); // Fixup planner notes. if (current_dive && d->id == current_dive->id) { diff --git a/qt-models/diveplannermodel.h b/qt-models/diveplannermodel.h index af958b938..1dadfacc7 100644 --- a/qt-models/diveplannermodel.h +++ b/qt-models/diveplannermodel.h @@ -131,8 +131,7 @@ private: void createTemporaryPlan(); struct diveplan diveplan; void computeVariationsDone(QString text); - void computeVariations(std::unique_ptr plan, const struct deco_state *ds); - void computeVariationsFreeDeco(std::unique_ptr plan, std::unique_ptr ds); + void computeVariations(struct diveplan plan, struct deco_state ds); // Note: works on copies of plan and ds int analyzeVariations(const std::vector &min, const std::vector &mid, const std::vector &max, const char *unit); struct dive *d; int dcNr;