Ultimaker / CuraEngine

Powerful, fast and robust engine for converting 3D models into g-code instructions for 3D printers. It is part of the larger open source project Cura.
https://ultimaker.com/en/products/cura-software
GNU Affero General Public License v3.0
1.68k stars 881 forks source link

Add option to command line settings to read in resolved values #2082

Closed casperlamboo closed 4 months ago

casperlamboo commented 4 months ago

Description

This PR adds an option to command line to slice a model file with resolved settings.

The json format of the file resolved settings is the following:

{
   "global": [SETTINGS],
   "extruder.0": [SETTINGS],
   "extruder.1": [SETTINGS],
   "model.stl": [SETTINGS]
}

where [SETTINGS] follow the schema

{
   [key: string]: bool | string | number | number[] | number[][]
}

There can be any number of extruders (denoted with extruder.n) and any number of models (denoted with [modelname].stl). The key of the model values will also be the filename of the relevant model, when running CuraEngine with this option the model file with that same name must be in the same folder as the resolved settings json.

To test this PR I've included an example containing the required resolved values json and model. Then, run CuraEngine with the following command.

CuraEngine slice -r ultimaker_s5.json -o benchy.gcode

Checklist:

NP-205

casperlamboo commented 4 months ago

I think we should put some of the documentation that is now in the PR about the format somewhere in the code. Otherwise you will likely lose that information if you ever want to figure out what it's trying to do (and forcing people to reverse engineer it)

Added documentation here https://github.com/Ultimaker/CuraEngine/pull/2082/commits/ed9004ae312aaf32bef5b2ff71ca0bfe412364cd

casperlamboo commented 4 months ago

Could you also please use less auto?

I use concrete types in all my function declarations since I see this as a form of documentation. I use auto in function bodies since

I really see no reason to change this..

but looking to a function signature to know the variable type is not really convenient.

not sure what you mean by this, could you elaborate?

wawanbreton commented 4 months ago
* most IDEs show type annotations anyway

I don't know how your IDE shows you more information, mine offers auto-completion even with auto but I can't know the type just by reading the code, or when reading it outside an IDE (like in a PR), and knowing the type is quite crucial to understand/check the code.

* I let the compiler decide on the type
* Am more flexible in programming since when changing a function signature no auto's need to be changed

IMHO, you should not. I agree that this can be very convenient when changing a signature, but it can lead to your compiler not using the type you actually meant, or with different attributes (const/pointer/reference), or in the worst case, generate unexpected type conversions.

I really see no reason to change this..

I won't push too hard for you to change it, but just pointing out that this may not be the best option overall. I read once that one should use auto only when the type can be deduced very easily, for example:

auto super_object = new MySuperClass();
auto other_object = reinterprect_cast<MySuperClass *>(input_object);

So I tend to keep following this rule. I also use it for cases when the actual type is really verbose but doesn't really matter, like for iterators in a loop:

for(auto iterator = my_list.begin() ; iterator != my_list.end() ; ++iterator)
{
    const MySuperClass *object = *iterator;
...

but looking to a function signature to know the variable type is not really convenient. not sure what you mean by this, could you elaborate?

When you see a piece of code like this:

const auto settings = readResolvedJsonValues(...

The actual type ofsettings is the return type of the function (hopefully), so the best way to know the type is to have a look at what the function returns. In worst cases, the function may also return auto which can become very annoying :sweat_smile: In any case, it breaks your "reading flow" and really doesn't help understanding.

I can't find an "official" recommandation about this, but what I read online tends to confirm this: use auto when the type is obvious or doesn't really matter, but avoid it when the type can't be easily deduced.

jellespijker commented 4 months ago

but it can lead to your compiler not using the type you actually meant, or with different attributes (const/pointer/reference), or in the worst case, generate unexpected type conversions.

auto always uses the copy operation. those different attributes therefore stil exist

auto ...
const auto ....
const auto& ...
auto& ...
auto* ....

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es11-use-auto-to-avoid-redundant-repetition-of-type-names

jellespijker commented 4 months ago

but it can lead to your compiler not using the type you actually meant, or with different attributes (const/pointer/reference), or in the worst case, generate unexpected type conversions.

auto always uses the copy operation. those different attributes therefore stil exist

auto ...
const auto ....
const auto& ...
auto& ...
auto* ....
jellespijker commented 4 months ago

also interesting read by Herb Sutter C++ ISO member

https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/

jellespijker commented 4 months ago

and if we require specific traits or functionality the concepts and requires can help.

ensuring the type has a certain trait