Skip to content
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.

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

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

Merged
merged 13 commits into from
May 28, 2024
Merged

Conversation

casperlamboo
Copy link
Contributor

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:

  • My code follows the style guidelines of this project as described in UltiMaker Meta
  • I have read the Contribution guide
  • I have commented my code, particularly in hard-to-understand areas
  • I have uploaded any files required to test this change

NP-205

Copy link
Member

@nallath nallath left a comment

Choose a reason for hiding this comment

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

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)

@casperlamboo
Copy link
Contributor Author

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 ed9004a

@jellespijker jellespijker self-requested a review May 23, 2024 16:46
Copy link
Member

@jellespijker jellespijker left a comment

Choose a reason for hiding this comment

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

LGTM;

Small tiny change suggestion.

src/MeshGroup.cpp Outdated Show resolved Hide resolved
Co-authored-by: Jelle Spijker <[email protected]>
Copy link
Contributor

@wawanbreton wawanbreton left a comment

Choose a reason for hiding this comment

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

Some minor remarks, otherwise makes sense.
Could you also please use less auto ? You can make aliases for types that are too long, but looking to a function signature to know the variable type is not really convenient.

include/geometry/LinesSet.h Outdated Show resolved Hide resolved
include/settings/types/LayerIndex.h Outdated Show resolved Hide resolved
include/slicer.h Outdated Show resolved Hide resolved
src/communication/CommandLine.cpp Outdated Show resolved Hide resolved
@casperlamboo
Copy link
Contributor Author

casperlamboo commented May 24, 2024

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

  • most IDEs show type annotations anyway
  • 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

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
Copy link
Contributor

* 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 😅 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
Copy link
Member

jellespijker commented May 24, 2024

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
Copy link
Member

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
Copy link
Member

also interesting read by Herb Sutter C++ ISO member

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

@jellespijker
Copy link
Member

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

ensuring the type has a certain trait

@jellespijker jellespijker mentioned this pull request May 28, 2024
8 tasks
@HellAholic HellAholic merged commit 3fb48b6 into main May 28, 2024
25 checks passed
@HellAholic HellAholic deleted the NP-205 branch May 28, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants