-
-
Notifications
You must be signed in to change notification settings - Fork 889
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
Conversation
NP-205
But delete copy operator/assignment instead NP-205
NP-205
There was a problem hiding this 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)
Added documentation here ed9004a |
There was a problem hiding this 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.
Co-authored-by: Jelle Spijker <j.spijker@ultimaker.com>
There was a problem hiding this 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.
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..
not sure what you mean by this, could you elaborate? |
I don't know how your IDE shows you more information, mine offers auto-completion even with
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 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:
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:
When you see a piece of code like this:
The actual type of I can't find an "official" recommandation about this, but what I read online tends to confirm this: use |
|
|
also interesting read by Herb Sutter C++ ISO member https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/ |
and if we require specific traits or functionality the ensuring the type has a certain trait |
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:
where
[SETTINGS]
follow the schemaThere 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.Checklist:
NP-205