-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[WIP] Add ability to set solid name when exporting STL #4986
base: master
Are you sure you want to change the base?
Conversation
This still needs to hook up to something that can read the file and determine the proper name
I like the movement! |
As mentioned many times before, using |
I'm not a fan of using $ variables here either, but mostly I consider that a design detail. I think the more obvious structure is to follow the pattern established by
Even that simple case presents a few problems: you have to convince the processing to not union them. (But what should happen if they overlap? The strongly-related problem of assigning color for multi-color prints really wants an answer to that question.) More complex cases lead quickly to constructs where I don't know what they mean:
Note that those cases are questions independent of whether the subtrees are designated using $ variables or modules or some new kind of annotation. What I would like to do is to find a solution for the "one part" problem, where we just want to control the internal name of an STL or the base file name of any export, in a way that will extend naturally to the multi-part case. |
It sounds like a section of the
This would set the solid name for the file, not worrying about the multi-part section. This should be able to be expanded to handle the |
There is no "setting" of values on geometry, there is no program running as in for example python. The And that's why I'm still not convinced nesting tree elements is a good idea. It certainly will work for geometry, but it can't distinguish between module declaration and module instantiation, covering only the latter. The problem with |
To be clear: Yes, it eats into global namespace. Ideally, a bunch of related concepts would be covered by that one module: color, material, separate sub-models to be printed separately, maybe slicing hints, et cetera, using named parameters. But it doesn't require new syntax. Any scheme would require either new syntax or eating into global namespace.
|
Allows the following syntax ``` part("part1") { sphere(3); } ``` Which will generate a `part1.<filename>.<extension>` (`part1.output.stl` for example)
I have a working example for The following
With a command line:
|
7efa012
to
93f9cc7
Compare
93f9cc7
to
f0dc250
Compare
While I'm still not convinced it's the best idea to use If this is going to include multi-file export (which would be very nice, even if it's initially for STL only!), please update the title of the PR and note that this will require some additional work as that PR will need to have at least some test cases and this scenario may not be fully covered by the test suite yet. I assume it's not a complicated thing, but still some extra topic to look at. |
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'm not sure if ".." is the best order of names. It feels like maybe swapping the first two would be better, so -o model.stl
would result in model.part1.stl
, model.part2.stl
.
|
||
Parameters parameters = Parameters::parse(std::move(arguments), inst->location(), {"name"}); | ||
|
||
const auto& solid_name = parameters["name"]; |
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 "name" is the better, more general choice here. I'd suggest dropping the "solid" part everywhere, although it's not critical as long as it's not visible to the user.
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 originally used name
but name()
is already used on the AbstractNode, resulting in a compile error.
@@ -2113,6 +2113,7 @@ ExportInfo createExportInfo(FileFormat format, const QString& exportFilename, co | |||
exportInfo.format = format; | |||
exportInfo.fileName = exportFilename.toLocal8Bit().constData(); | |||
exportInfo.displayName = exportFilename.toUtf8().toStdString(); | |||
exportInfo.solidName = "fromMainWindowcc"; |
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.
What's that?
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.
This was a holdover from when I was debugging how exporting works. All my testing is in the CLI path and I have not done any testing exporting from the GUI yet. This is in the path of the GUI export and will probably be removed once I start getting to GUI exports.
src/io/export_stl.cc
Outdated
{ | ||
// FIXME: In lazy union mode, should we export multiple solids? | ||
if (binary) { | ||
char header[80] = "OpenSCAD Model\n"; | ||
output.write(header, sizeof(header)); | ||
output << solidName << "\n"; |
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.
As this is a user specified value, this must be cleaned and size limited to the allowed 80 characters header. This also should be checked to not start with "solid " as that's the beginning of ASCII STL.
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.
Would we want to limit the solid name to 80 characters for both binary and ascii STL?
src/openscad.cc
Outdated
|
||
std::string solid_name = "OpenSCAD_Model"; |
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.
This needs to go to a place that can be called later from GUI too. It's ok if we start with command line support, but it needs to anticipate that this same feature will be available from GUI at some point without rewriting everything.
} | ||
|
||
|
||
int render_and_export( |
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.
If multiple output files are generated, that needs to be available for the dependency generator too (the -d
command line parameter).
I wonder if that may even be broken currently as I''m not sure this is covered by test cases.
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.
Is there anything special I need to do to make them available? Where do I tell it the filename?
i think for binary stl you definitely have to terminate with '\0' instead
of newline, else the newline becomes part of the solid name, which might
confuse other programs,
…On Wed, Feb 21, 2024 at 4:30 AM Joshua Milas ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/io/export_stl.cc
<#4986 (comment)>:
> char tmp_triangle_count[4] = {0, 0, 0, 0}; // We must fill this in below.
output.write(tmp_triangle_count, 4);
} else {
setlocale(LC_NUMERIC, "C"); // Ensure radix is . (not ,) in output
- output << "solid OpenSCAD_Model\n";
+ output << "solid " << solidName << "\n";
I set the limit to 80 characters, 79 + newline, for both binary and ascii.
I also filter out whitespace characters.
—
Reply to this email directly, view it on GitHub
<#4986 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCO4MUEAKSYWSQHKZC5THTYUVS5JAVCNFSM6AAAAABDILXRY6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJSGAYTINBUGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
The header needs to be 80 characters long acording to the spec
I added the suggested changes and left comments on the ones still open. I would like to see if I can export just one STL with multiple parts, it seems I have to modify I fixed most of the build errors, there is now just one on windows. Ill have to try to cross-compile and run the tests to see whats going on there. @gsohler The original code had a newline after the name followed by null characters, this follows the same [now]. Should the newline be removed? |
When I last thought about this, I believe I was thinking of a two-level namespace, where the arguments to This requires #4478 for a user-space syntax for creating objects. |
Do you have any consumers that can read such a file? |
I have been using OpenFOAM to do simulations that consume STL files. I have tested multi-region with ASCII STL by |
Hi DeepHorizons, |
@gsohler I would want to try it out but it sounds like it could work. |
I just like to mention that there is a distinction between Objects and Parts - while the 3mf only know "objects" there are objects with in a model file (parts) and separate object files (objects) So it will be important to allow hierarchy to group parts to objects and have multiple objects. |
Attempt at #4977
Still need to hook this up to something that will determine the solid name. As suggested in the issue a special variable could be set,
$solid_name
, but I'm not sure where to look to start modifying that.