Skip to content

CURA-13049 Add a Reusable ChainableEnvironment and optimize the EnvironmentMap #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wawanbreton wants to merge 3 commits into
base: main
Choose a base branch
from
Open

CURA-13049 Add a Reusable ChainableEnvironment and optimize the EnvironmentMap #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6140842
Add a Reusable ChainableEnvironment and optimize the EnvironmentMap
wawanbreton Mar 31, 2026
a6bc213
Add possibly missing header
wawanbreton Apr 28, 2026
8f6f71f
Pass maps by reference to avoid copy
wawanbreton Apr 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion conandata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
version: "1.0.0"
version: "1.1.0"
67 changes: 49 additions & 18 deletions include/cura-formulae-engine/ast/ast.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
#pragma once

#include "cura-formulae-engine/eval.h"

#include <zeus/expected.hpp>

#include <functional>
#include <string>
#include <unordered_map>
#include <unordered_set>
#include <zeus/expected.hpp>

#include "cura-formulae-engine/eval.h"

Comment thread
wawanbreton marked this conversation as resolved.
namespace CuraFormulaeEngine::eval
{
Expand Down Expand Up @@ -36,10 +36,13 @@ class EnvironmentMap : public Environment
{
private:
std::unordered_map<std::string, eval::Value> environment_ = {};
public:

public:
EnvironmentMap() = default;
explicit EnvironmentMap(const std::unordered_map<std::string, eval::Value>& map) : environment_(map) {}
explicit EnvironmentMap(const std::unordered_map<std::string, eval::Value>& map)
: environment_(map)
{
}
~EnvironmentMap() override = default;

[[nodiscard]] std::optional<eval::Value> get(const std::string& key) const noexcept override;
Expand All @@ -52,31 +55,58 @@ class EnvironmentMap : public Environment

void set(const std::string& key, const eval::Value& value) noexcept;

[[nodiscard]] EnvironmentMap clone() const noexcept;
void add(const std::unordered_map<std::string, eval::Value>& values);

[[nodiscard]] EnvironmentMap clone() const noexcept;
};

class LocalEnvironment : public Environment
class ChainableEnvironment : public Environment
{
private:
const Environment* shadow_environment_{ nullptr };

public:
explicit ChainableEnvironment(const Environment* shadow_environment = nullptr)
: shadow_environment_(shadow_environment)
{
}
~ChainableEnvironment() override = default;

LocalEnvironment() = default;
explicit LocalEnvironment(const Environment* shadow_environment_)
: shadow_environment_(shadow_environment_)
[[nodiscard]] std::optional<eval::Value> get(const std::string& key) const noexcept override final;

[[nodiscard]] bool has(const std::string& key) const noexcept override final;

[[nodiscard]] std::unordered_map<std::string, eval::Value> getAll() const noexcept override final;

protected:
[[nodiscard]] virtual std::optional<eval::Value> getImpl(const std::string& key) const noexcept = 0;

[[nodiscard]] virtual bool hasImpl(const std::string& key) const noexcept = 0;

[[nodiscard]] virtual std::unordered_map<std::string, eval::Value> getAllImpl() const noexcept = 0;
};

class LocalEnvironment : public ChainableEnvironment
{
private:
EnvironmentMap local_environment_;

public:
explicit LocalEnvironment(const Environment* shadow_environment = nullptr)
: ChainableEnvironment(shadow_environment)
{
}
~LocalEnvironment() override = default;

EnvironmentMap local_environment_ = {};
const Environment* shadow_environment_;

[[nodiscard]] std::optional<eval::Value> get(const std::string& key) const noexcept override;
[[nodiscard]] std::optional<eval::Value> getImpl(const std::string& key) const noexcept override;

[[nodiscard]] bool has(const std::string& key) const noexcept override;
[[nodiscard]] bool hasImpl(const std::string& key) const noexcept override;

[[nodiscard]] std::unordered_map<std::string, eval::Value> getAll() const noexcept override;
[[nodiscard]] std::unordered_map<std::string, eval::Value> getAllImpl() const noexcept override;

void set(const std::string& key, const eval::Value& value);

void add(const std::unordered_map<std::string, eval::Value>& values);
};
Comment thread
wawanbreton marked this conversation as resolved.

} // namespace CuraFormulaeEngine::env
Expand Down Expand Up @@ -123,7 +153,8 @@ struct Expr
[[nodiscard]] virtual bool deepEq(const Expr& other) const = 0;

/**
* @brief Traverses the expression tree and applies the visitor function to each node.
* @brief Traverses the expression tree and applies the visitor function to
* each node.
*
* @param visitor The visitor function to apply to each node.
*/
Expand Down
68 changes: 46 additions & 22 deletions src/ast/ast.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ namespace CuraFormulaeEngine::env

std::optional<eval::Value> EnvironmentMap::get(const std::string& key) const noexcept
{
if (!has(key))
auto iterator = environment_.find(key);
if (iterator == environment_.end())
{
return std::nullopt;
}
return environment_.at(key);
return iterator->second;
}

bool EnvironmentMap::has(const std::string& key) const noexcept
Expand All @@ -32,44 +33,67 @@ void EnvironmentMap::set(const std::string& key, const eval::Value& value) noexc
environment_.insert_or_assign(key, value);
}

void EnvironmentMap::add(const std::unordered_map<std::string, eval::Value>& values)
{
environment_.insert(values.begin(), values.end());
Comment thread
wawanbreton marked this conversation as resolved.
}

EnvironmentMap EnvironmentMap::clone() const noexcept
{
return EnvironmentMap{environment_};
return EnvironmentMap{ environment_ };
}

std::optional<eval::Value> LocalEnvironment::get(const std::string& key) const noexcept
std::optional<eval::Value> LocalEnvironment::getImpl(const std::string& key) const noexcept
{
if (local_environment_.has(key))
{
return local_environment_.get(key);
}
if (shadow_environment_ && shadow_environment_->has(key))
return local_environment_.get(key);
}

bool LocalEnvironment::hasImpl(const std::string& key) const noexcept
{
return local_environment_.has(key);
}

std::unordered_map<std::string, eval::Value> LocalEnvironment::getAllImpl() const noexcept
{
return local_environment_.getAll();
}

void LocalEnvironment::set(const std::string& key, const eval::Value& value)
{
local_environment_.set(key, value);
}

void LocalEnvironment::add(const std::unordered_map<std::string, eval::Value>& values)
{
local_environment_.add(values);
}

std::optional<eval::Value> ChainableEnvironment::get(const std::string& key) const noexcept
{
std::optional<eval::Value> value = getImpl(key);
if (! value.has_value() && shadow_environment_)
{
return shadow_environment_->get(key);
value = shadow_environment_->get(key);
}
return std::nullopt;
return value;
}

bool LocalEnvironment::has(const std::string& key) const noexcept
bool ChainableEnvironment::has(const std::string& key) const noexcept
{
return local_environment_.has(key) || (shadow_environment_ && shadow_environment_->has(key));
return hasImpl(key) || (shadow_environment_ && shadow_environment_->has(key));
}

std::unordered_map<std::string, eval::Value> LocalEnvironment::getAll() const noexcept
std::unordered_map<std::string, eval::Value> ChainableEnvironment::getAll() const noexcept
{
std::unordered_map<std::string, eval::Value> all;
if (shadow_environment_) {
if (shadow_environment_)
{
all = shadow_environment_->getAll();
}

auto scoped = local_environment_.getAll();
auto scoped = getAllImpl();
all.insert(scoped.begin(), scoped.end());
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChainableEnvironment::getAll() builds the result by starting with the shadow environment and then calling all.insert(scoped.begin(), scoped.end()). insert does not overwrite existing keys, so any key present in the shadow will prevent the local/scoped value from being included. This contradicts get()/has() behavior where the local environment should shadow the outer one. Use an overwrite merge strategy (e.g., insert-or-assign for scoped entries, or build from scoped then add missing shadow entries).

Suggested change
all.insert(scoped.begin(), scoped.end());
for (const auto& [key, value] : scoped)
{
all.insert_or_assign(key, value);
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@casperlamboo casperlamboo Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree with the suggestion here, why not use insert_or_assign to assure scoped will override the shadow env? I do realize that this was the code before, just wondering if this was ever the right logic

Copy link
Copy Markdown
Contributor Author

@wawanbreton wawanbreton Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the overriding ordering was never a real question, since the keys in each chained environment are in practice mutually exclusive. We could make it an actual topic if required, but I don't really see a need yet. That would also imply writing documentation to make it explicit.

Copy link
Copy Markdown
Contributor

@casperlamboo casperlamboo Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree that keys are always mutually exclusive, I can see a ton of use cases where global stack is shadowing an extruder stack for instance and both stacks have a ton of duplicate keys. For me the naming ShadowEnvoronment makes it very much expected that the variables from this env will be overwritten.

Copy link
Copy Markdown
Contributor Author

@wawanbreton wawanbreton Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree that the same setting can be in both the extruder settings and the global settings, however this is handled internally in CuraEngine by having the Settings objects chained together. So in the end, when asking for a value in the extruders settings adapter, it will call the global Setting objects if the extruder Setting has no such key. However the adapter containing the global Setting is not linked to the adapter containing the extruders Settings
I added a diagram in the engine documentation to get an overview of the adapters chain:

Untitled Diagram drawio

Copy link
Copy Markdown
Contributor

@casperlamboo casperlamboo Apr 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then the current logic is still non-expected right and thus error-prone? I can imagine situations in either curaengine or curator where we would rely on this behavior. I believe we're currently not relying on this behavior but it would be a pain to debug.

And if we don't want the for-loop we can also adjust the ordering of both insertion-blocks.

Copy link
Copy Markdown
Contributor Author

@wawanbreton wawanbreton Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed, the current logic is inconsistent and could lead to bugs in the future. I will do the changes to improve it.

Comment thread
wawanbreton marked this conversation as resolved.
return all;
}

void LocalEnvironment::set(const std::string& key, const eval::Value& value)
{
local_environment_.set(key, value);
}

} // namespace CuraFormulaeEngine::env
} // namespace CuraFormulaeEngine::env
Loading