CURA-13049 process the end variable replacement in the engine#2313
CURA-13049 process the end variable replacement in the engine#2313wawanbreton wants to merge 34 commits intomainfrom
Conversation
…-the-end-variable-replacement-in-the-engine
Test Results31 tests 31 ✅ 5s ⏱️ Results for commit 13f1704. ♻️ This comment has been updated with latest results. |
CURA-13049 Calculating the initial extruder number on engine side allows fixing a few issues where the pre-supposed initial extruder ended up not being the same as actually generated by the engine. Now we calculate it after slicing, so we are really sure this is the correct one, then send it to the front-end.
CURA-13049 Code was already correct regarding previous behavior, see https:// github.com/Ultimaker/Cura/blob/2cb536439c84ec5f4169008c5682a75d7d6db11a/ plugins/CuraEngineBackend/CuraEngineBackend.py#L876
CURA-13049 In a worst-case-ish scenario, in debug mode, I measured a total resolving time of 600ms. This time was cut down by half with the multi- threading, so good to take. The GCodeTemplateResolver has been designed so that the resolving is a const method, that can be safely be called in parallel.
…ent-in-the-engine
…-the-end-variable-replacement-in-the-engine
9c354e8 to
b45f19f
Compare
…e-end-variable-replacement-in-the-engine
There was a problem hiding this comment.
Pull request overview
This PR refactors CuraEngine’s GCode templating pipeline so end-of-slice variables can be resolved inside the engine by buffering GCode parts in memory, then emitting them via the Communication interface at the end of slicing. It also expands the data reported to the front-end with initial extruder, bounding boxes, and richer material usage statistics.
Changes:
- Buffer fixed/resolvable GCode as
GCodePartobjects and resolve/send them at end of slicing viaCommunication::sendGCodePart. - Replace legacy “begin/flush gcode” communication hooks with
sendGCodePart+sendPrintInformation, and thread print/material information through the Communication implementations. - Add new print/material metadata (
PrintInformation, material length/weight/cost/name, initial extruder, initial-layer BB) and extend the protobuf schema.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/arcus/MockCommunication.h | Updates Communication mocks for new send APIs (currently mismatched). |
| tests/arcus/ArcusCommunicationTest.cpp | Adapts Arcus tests to sendGCodePart semantics. |
| tests/GCodeTemplateResolverTest.cpp | Updates template resolver tests for new resolver object/contexts. |
| tests/GCodeExportTest.cpp | Adjusts export tests to consume buffered GCodePart output instead of a stream. |
| src/settings/SettingContainersEnvironmentAdapter.cpp | Switches adapter to chainable environment + new impl hooks. |
| src/pathPlanning/NozzleTempInsert.cpp | Fixes include path to new gcode_export/gcodeExport.h. |
| src/gcode_export/gcodeExport.cpp | Implements buffering, final resolve/send, and material/bounding-box variables. |
| src/gcode_export/ResolvedGCodePart.cpp | New: deferred template resolution GCode part. |
| src/gcode_export/GcodeTemplateResolver.cpp | Refactors resolver into stateful class with prepared environments. |
| src/gcode_export/FixedGCodePart.cpp | New: buffered fixed-string GCode part with its own stream. |
| src/communication/EmscriptenCommunication.cpp | Moves engine-info sending to ctor; adds sendPrintInformation path. |
| src/communication/CommandLine.cpp | Writes output via sendGCodePart and manages file output stream. |
| src/communication/ArcusCommunication.cpp | Replaces flush/begin with sendGCodePart; enriches print info messaging. |
| src/Slice.cpp | Moves processor finalize into Slice::compute(). |
| src/Scene.cpp | Removes per-meshgroup flushGCode calls. |
| src/PrintInformation.cpp | New: collects initial extruder and initial-layer bounding box. |
| src/PrimeTower/PrimeTower.cpp | Updates include path to new gcode export header location. |
| src/PrimeTower.cpp | Updates include path to new gcode export header location. |
| src/LayerPlanBuffer.cpp | Removes flushGCode calls while processing/flush layers. |
| src/LayerPlan.cpp | Adds helpers to compute initial extruder and extrusion BB for a layer. |
| src/FffProcessor.cpp | Removes facade methods that exposed GCodeWriter internals. |
| src/FffGcodeWriter.cpp | Removes begin/stream facade; accumulates PrintInformation during slicing. |
| src/ExtruderPlan.cpp | Adds extrusion detection + extrusion bounding box computation (has a crash bug). |
| include/settings/SettingContainersEnvironmentAdapter.h | Updates adapter base class and override API. |
| include/gcode_export/gcodeExport.h | Adds buffered GCode parts, resolver context, new finalize signature. |
| include/gcode_export/ResolvingExtruderContext.h | New: variant-based extruder context for deferred resolution. |
| include/gcode_export/ResolvedGCodePart.h | New: deferred-resolve part interface. |
| include/gcode_export/GcodeTemplateResolver.h | New: stateful resolver class API (header missing std includes). |
| include/gcode_export/GCodePart.h | New: base class for buffered GCode parts. |
| include/gcode_export/FixedGCodePart.h | New: fixed-string part API. |
| include/communication/EmscriptenCommunication.h | Updates API to sendPrintInformation and new slice-info message creation. |
| include/communication/Communication.h | Replaces begin/flush with sendGCodePart; replaces estimates with sendPrintInformation. |
| include/communication/CommandLine.h | Adds output stream/file members (header missing std includes). |
| include/communication/ArcusCommunication.h | Updates interface overrides to new send methods. |
| include/PrintInformation.h | New: print/material metadata structures. |
| include/LayerPlan.h | Declares new layer helpers for initial extruder + extrusion BB. |
| include/GcodeTemplateResolver.h | Removes old free-function resolver header. |
| include/FffProcessor.h | Removes facade methods for file/stream + stats access. |
| include/FffGcodeWriter.h | Removes file/stream facade, adds PrintInformation member. |
| include/ExtruderPlan.h | Declares new extrusion helpers. |
| conandata.yml | Bumps cura-formulae-engine dependency version. |
| Cura.proto | Extends material estimates + adds InitialExtruder message. |
| CMakeLists.txt | Updates source list to new gcode_export files + PrintInformation. |
Comments suppressed due to low confidence (2)
src/gcode_export/gcodeExport.cpp:1984
- This code uses ranges::views::enumerate and ranges::views::drop but the file doesn't include any range-v3 headers. Add the appropriate range-v3 includes (e.g. view/enumerate.hpp, view/drop.hpp, and any pipe support) or replace these loops with standard indexed loops to avoid build failures due to missing declarations.
src/gcode_export/gcodeExport.cpp:1995 - filaments_amounts (exported as the "filament_amount" variable) is currently populated with extruder_info.filament_length, while ExtruderPrintInformation::filament_amount is documented/used as volume (mm^3). This looks like a units mix-up that will cause incorrect values in resolved GCode; populate filament_amount with filament_amount (volume) or rename the variable if the intent is length.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e-end-variable-replacement-in-the-engine
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'C++ Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 13f1704 | Previous: 7300d08 | Ratio |
|---|---|---|---|
SimplifyTestFixture/simplify_local |
1.9423783240468322 ns/iter |
1.2484627420333818 ns/iter |
1.56 |
This comment was automatically generated by workflow using github-action-benchmark.
|
|
||
| // Create an environment containing all the extra global settings | ||
| global_environment_ = std::make_shared<cfe::env::LocalEnvironment>(&CuraFormulaeEngine::env::std_env); | ||
| global_environment_->add(extra_global_settings); |
There was a problem hiding this comment.
Depending on the size of extra_global_settings you can also consider not using the add function (and copying all values) but instead introduce two layers of scoping.
There was a problem hiding this comment.
That is true, but in practice there is always global settings when calling this method, so unless you want to re-use it somewhere else, it would just add some extra-complexity here.
Not 100% sure I understand your remark though 🤔
There was a problem hiding this comment.
Not sure if we have the right "adpaters" for this, but instead of doing this
global_environment_ = std::make_shared<cfe::env::LocalEnvironment>(&CuraFormulaeEngine::env::std_env);
global_environment_->add(extra_global_settings);
global_environment_->set("initial_extruder_nr", static_cast<int64_t>(initial_extruder_nr_));Do this
global_environment_ = std::make_shared<cfe::env::LocalEnvironment>(
ScopedEnvironment(EnvironmentMap(extra_global_settings), CuraFormulaeEngine::env::std_env)
);
global_environment_->set("initial_extruder_nr", static_cast<int64_t>(initial_extruder_nr_));Where EnvironmentMap takes two Envionments and considers the second argument to be shadowing the first. Similar to LocalEnvironment the result is itself an Envionment. Benifit is that there is less overhead constructing the global_environment_ since we're not copying values. Downside is that lookups are more expensive since we need to go through 3 layers of environments in the worst case instead of 2.
There was a problem hiding this comment.
At some point I have been considering making an EnvironmentChain that could contain any number of chained environment, but that is not really possible in the end because the chain looks more like a graph.
About the performance, the prepareForResolving method is called only once in the whole engine processing, so I do think the cost of copying the extra settings is quite negligible. Also I don't see how your method avoids copying them 🤔. If we really want to avoid that, it could be given as a move-able variable.
There was a problem hiding this comment.
You avoid copying since you're not calling add which copies all values from extra_global_settings and instead use the extra_global_settings as-is. But I agree benifits are limited
There was a problem hiding this comment.
From what I can see, the extra_global_settings would be copied in the EnvironmentMap constructor anyway. But never mind, let's keep it as is.
CURA-13049 Co-authored-by: Casper Lamboo <c.lamboo@ultimaker.com>
This PR proposes a few changes to process the end-of-line variables replacement in the engine. It implements the following changes:
FixedGCodePartinstanceResolvedGCodePartinstanceCommunicationobject to send the different pieces of information at the proper times, so that the proces is more repeateable across different implementations, and this also removes some facade code in theFffProcessorandFffGCodeWriter.ℹ️ Some technical notes:
EmscriptenCommunicationhas been updated to reach feature parity. However it is to note that theCommunication::beginGCodemethod has been removed, and its content has been placed in the constructor instead. The previous behavior can be restored if necessary.Since this is now easily possible, it also adds a few useful variables to the resolvable GCode:
initial_layer_bb_{min_x, max_x, min_y, max_y, width, height}containing the 2D bounding box of the effective print on the first layer (this includes every helper e.g. brim, skirt, prime tower, ...)total_bb_{min_x, max_x, min_y, max_y, min_z, max_y, width, depth, height}containing the 3D bounding box of the models to be printedis_extruder_usedcontaining a list of 16 booleans, indicating which extruder is effectively used during the printRequires Ultimaker/Cura#21500
Requires Ultimaker/CuraFormulaeEngine#2
CURA-13049