search

slic3r / Slic3r

Open Source toolpath generator for 3D printers
https://slic3r.org/
GNU Affero General Public License v3.0
3.32k stars 1.29k forks source link

[GSoC] Samir55 / 3MF file format #3934

Closed alranel closed 6 years ago

alranel commented 7 years ago

This issue is dedicated to the GSoC student @Samir55 who proposed to implement support for the 3MF file format in Slic3r: https://summerofcode.withgoogle.com/projects/#6343944127381504

This topic was discussed in this original issue:

2811

@Samir55 already contributed to the Slic3r codebase by adding zoom commands (pull request #3864, merged).

@Samir55, welcome to Slic3r! Would you please introduce yourself to the Slic3r community? Please mention any experience with 3D printing or similar technologies, and include your time zone and operating system.

Samir55 commented 7 years ago

Dev Log(15 - June):

Samir55 commented 7 years ago

What's left in 3MF Write Function:

Samir55 commented 7 years ago

@alexrj @lordofhyphens I am working on getting the transformation matrix from scale, X and Y translation and z-axis rotation attributes found in ModelInstance as 3MF item element which represents ModelInstance is declared like this:

<item objectid="4" transform="5.96045e-008 0.999997 0 -0.999997 -9.40395e-007 0 0 0 1 126.999 -126.998 0.00197"/>
lordofhyphens commented 7 years ago

Yeah using a 3rd program that should be competently made will give you a useful crosscheck.

Samir55 commented 7 years ago

Dev Log(15 - June) II:

@alexrj @lordofhyphens I have opened 2 exported 3MF files on Microsoft 3D Builder. Here are some screenshots astl magnoliaobj

Commits:

Commit : Fix ContentTypes.xml file causing 3D Builder not to load the file. xmlns schema link added to < Types > element. Commit : Initialize partnumber to the ModelObject constructors. Commit : Remove the first extension from the exported file name. when I export to 3mf the resulted file name was m.obj.3mf. @lordofhyphens is there a reason for keeping this as 3D builder cannot load file with such name.

lordofhyphens commented 7 years ago

There is no reason to add an extra "obj" in the file name that I am aware of.

alranel commented 7 years ago

@Samir55, I really appreciate your dev log. Very good documentation of your progress. Do you have any open questions?

Samir55 commented 7 years ago

@alexrj Thank you :)

Samir55 commented 7 years ago

Dev Log(19 - June):

Write Slic3r custom configs data to ModelMaterials, ModelObject, ModelVolumes. I have done this according to this structure:

"<slic3r:material mid=\"" + to_string(material_index)
                                  + "\" type=\"" + key + "\">"
                                  + material.second->config.serialize(key) 
                                  +   "</slic3r:material>\n"

where "mid" is the index of the basematerial it points to from the group.

Commits :

@lordofhyphens @alexrj if you have any comments , please tell me.

Samir55 commented 7 years ago

Dev Log(20 - June):

lordofhyphens commented 7 years ago

Any particular reason why you are including the producing version?

So our volumes (primarily used as modifiers) would be ignored by other 3mf reading programs, then?

Consider a refactor to translate materials between 3mf and amf as a stretch goal. I wouldn't do much about it now.

So long as it works, meets the general requirements (preferably header-only, etc), and doesn't turn into a giant timesink, I don't think @alexrj or I care much about you rolling your own zip implementation. My suggestion is if you're going to go that route, make sure that the interface doesn't change from the wrapper you are using now. It will make debugging easier and you can deal with it after getting the kinks worked out of the main effort.

Any other open questions?

Samir55 commented 7 years ago

@lordofhyphens

lordofhyphens commented 7 years ago

@Samir55

lordofhyphens commented 7 years ago

It came from IRC #slic3r....

[09:43] <Samir> Zip lib compiles fine with cmake (it works) and slic3r builds fine with Build.pl on mac os but I have encountered this error on Ubuntu 14.04 when running Build.pl, Here it is (https://pastebin.com/r5jQs4ep)
[09:44] <Samir> LoH: Can this be error be handled without changing in zip lib code?
[09:45] <LoH|Work> so is zip.c actually required to use the library?
[09:46] <Samir> LoH|Work : yes the Zip lib consists of (zip.h, zip.c and miniz.h)
[09:47] <LoH|Work> so there's part of the implementation in zip.c?
[09:48] <Samir> LoH|Work, yes the zip.c file is here (https://github.com/kuba--/zip/blob/master/src/zip.c)
[09:48] <LoH|Work> It looks like it is shadowing something in your standard include libs, so try removing the declaration and see if it breaks ;)
[09:49] <LoH|Work> I don't know offhand why it would compile with cmake and not with build.pl other than the cmake build may have different flags.
[10:43] <Samir> LoH|Work: I removed the words static and const from basename funtion declaration and it has worked , However on macOS Build.pl works fine without any modification and does not give any errors when compiling zip.c
[10:43] <LoH|Work> I'm not surprised, the conflict was with a system include libraryh
[10:44] <LoH|Work> it would probably only break on that or similar enough linux distros
[10:45] <Samir> LoH|Work: what do you recommend to do in this case?
[10:47] <LoH|Work> tweak zip.c. The problem will go away if you roll your own zip lib later.
[10:50] <Samir> LoH|Work : I will start implementing it today . I was only giving a last chance for including it. What is left in TMF::read is some refactoring and supporting Material Extension. Thanks LoH :)
Samir55 commented 7 years ago

Question:

Should I throw exceptions or leave return false in the functions if there was any error, for example, cannot open a file, cannot write to file, etc?

lordofhyphens commented 7 years ago

What does the AMF reader do?

Samir55 commented 7 years ago

@lordofhyphens It doesn't throw exceptions. O.K. I will continue like in AMF.

Samir55 commented 7 years ago

Dev Log(22 - June):

Dev Log(20,21 - June):

There was not code at those days I spent considerable time doing the following:

Samir55 commented 7 years ago

@alexrj I need some overall code review , I haven't finished yet but if you see something that needs improvements tell me.

Samir55 commented 7 years ago

Slic3r Material Extension Summary (I)

screen shot 2017-06-24 at 12 58 21 am

A. Color group:

// b. Add material group id to ModelMaterial Class. int material_group; ///< The material belong to which group. Specific to 3MF file format. By default = 0 which means it's a base material or AMF material. / I assume 1: means 3MF basematerials or AMF read materials. 2: means color group. 3:means composite materials. 4: means multiproperties. /

// c. Write color groups to the write_materials() function in TMF editor class.

    for (const auto &material : model->color_group) {
        if(!color_group_written){
            append_buffer("<m:colorgroup id=\"2\">\n");
            color_group_written = true;
        }
        append_buffer("<m:color color=\"" + material.second->attributes["color"] + "\" />\n");
    }

    // Close the material color group if it's open.
    if(color_group_written)
        append_buffer("</m:colorgroup>\n");

#### B. Composite Materials:
@alexrj @lordofhyphens Are composed materials supported in Slic3r ? Also, Do we need to support all those kinds of materials beside base materials in core specification?
* Composite Material is a mixture of 2 or more base materials.
* Currently I am working on implementing them like color group.

#### C. Texture 2d Group: I will ignore that group.
lordofhyphens commented 7 years ago

Currently we wouldn't do anything with materials. Treating them like colors is probably fine.

Samir55 commented 7 years ago

Dev Log(23 - June):

std::map<int, std::string> material_groups_types; // The material group id is mapped to its type whether it's a color, composite materials or multi properties group.

lordofhyphens commented 7 years ago

@Samir55 I looked through your branch. My comments here going to be "craftsmanship" style comments.

Please pay more attention to what your editor is doing to whitespace and what you commit to your branch. I was browsing over the changes you committed to slic3r.cpp and noticed that they are all just to whitespace (and not productively). This tends to lead to more headaches when performing merges later.

Some of your comments are seemingly redundant in TMF.cpp "Constructor" is not, in my opinion, a useful code comment (that it is a constructor should be obvious from the method's signature). Same with "Convert to string" as a comment on a to_string method is not a useful comment; explaining a special method for determing the string representation would be.

Is there a reason that TMFEditor is a struct and not a class? zip_archive does require going through the class to do anything meaningful with it, so leaving a naked pointer out like that is not particularly good practice.

Is there a reason the template functions of TMFEditor are in the .cpp file (and that there is no header for it)? IIRC nonspecialized templates are a bit wonky in non-header files (and we don't like #include cpp)

Does the 3mf-specific part number really need to be included as an class variable in Model? It's not necessarily wrong, I would like you to explain your reasoning as a confirmation that it is necessary ;) Is there enough information elsewhere to reconstruct it for export, or is it being included just for reflection purposes (export of imported file is same)?

Same with the color groups, etc.

lordofhyphens commented 7 years ago

@Samir55 if you open a pull request you can get the build server(s) to run your code.

You should probably pause implementing features and devise some basic I/O tests for each feature you have implemented so far, ensuring that they are read appropriately from a sample file and then written (and read), either as unit tests for TMFEditor or at the very minimum high level test cases for read() and write().

Samir55 commented 7 years ago

@lordofhyphens Thanks for your review :)

I have asked Sound about this and he asked:

interesting. so they are unique identifiers we should keep attached to objects.
let's add a property to ModelObjects, and make sure you document they are used for the 3MF part numbers

Dev Log (24 - June) I:

Commits solving the points above:

lordofhyphens commented 7 years ago

@Samir55 A struct has all of its members public by default; this represents the interface of TMFEditor. What I would like you to do is put some thought and consideration into the interface (what should be public and what should be private).

Are functions not called by TMFEditor supposed to be messing with the contents of the pointer? If not, zip_archive should be private.

Samir55 commented 7 years ago

@lordofhyphens Okay I will take this into consideration :).

lordofhyphens commented 7 years ago

Anything that is exposed via the public interface should be unit tested at a minimum.

Samir55 commented 7 years ago

Dev Log(24 - June) II:


* I have made only the 2 functions produce_TMF() and consume_TMF() only public functions and those are the ones I am going to implement unit tests for them.
* [Commit I](https://github.com/alexrj/Slic3r/pull/4046/commits/87b63eaf6c91d521953cffdab00e219674a88f1a)
Samir55 commented 7 years ago

Dev log(25, 26 - June):

Error:

Modified or Added Files:

Samir55 commented 7 years ago

Dev Log(27 - June):

Samir55 commented 7 years ago

Dev Log(28- June):

Samir55 commented 7 years ago

Dev Log (30 - June):

Samir55 commented 7 years ago

Dev Log(1 - July):

lordofhyphens commented 7 years ago

A. Slic3r tends to assume everything is in mm, but it is generally unit less (so long as everything has the same units).

B. Origin point on a plate export is 0,0. On a solo model export I believe it is what was read in.

C. It is there to tell the compiler how to call functions (basically plumbing). You shouldn't have to use it yourself explicitly or write something similar.

D. Seems fine to me.

Samir55 commented 7 years ago

Dev Log(2 - July):

Samir55 commented 7 years ago

Dev Log(4- July):

Samir55 commented 7 years ago

Dev Log(5 - July):

Samir55 commented 7 years ago

What's left in TMF::read()

Today, I will be working on decomposing the affine 3d transformation matrix.

Materials Problem

The problem in model materials is:

std::map<string, ModelMaterial*> // (material_id, material pointer)

The current implementation

/// In Model class.
++ std::vector<std::pair<int, ModelMaterialMap>> material_groups;
++    ///< It's a vector carrying pairs each has the material group type and its materials.

then we will be able to use that to read and write materials without much pain but this will need handling that change when writing AMF.

  1. Continue using the current implementation but beside adding the material to it's material_gorup add it to the original ModelMaterials map (materials) and when we exporting to AMF we can ignore those material groups and accept base materials basematerials are like materials in amf.

  2. Ignore the current implementation and read only basematerials and ignore the material extension groups, But this may in the future cause crash because of integer overflow in case of large number of base materials

@alexrj @lordofhyphens what's the best way to handle this?

Current output.

(I haven't yet applied the transformation to the model instances).

Samir55 commented 7 years ago

Dev Log( 8 - July):

Dev Log( 6- July):

Samir55 commented 7 years ago

Dev Log( 9 - July):

Commits: Commit 1, Commit 2 I only did as the code found in this link "ETdoFresh answer" @lordofhyphens Is this method correct? It's as follows:

Samir55 commented 7 years ago

Dev Log( 11 - July):

Samir55 commented 7 years ago

Dev Log (12 - July):

Samir55 commented 7 years ago

@alexrj @lordofhyphens I have a question regarding reading the item tag (which corresponds to creating the model instance of an object). I extracted the following parameters from the matrix in the item tag:

While what is found in ModelInstance:

3MF specification requires (is a must) applying the matrix. Do you recommend me to add those parameters to the ModelInstance or what's the case?

Samir55 commented 7 years ago

For reading the transformation matrix in component tag (An object is a part of a bigger object) desn't have the previous problem as I create a copy of the component object apply rotation, translation and scale to the copied object then get the mesh and add it to volumes of the big object.


                    // Create a copy of the current object.
                    ModelObject* object_copy = m_model.add_object(*component_object, true);

                    apply_transformation(object_copy, transformations);

                    // Get the mesh of this object.
                    component_mesh = object_copy->mesh();

                    // Delete the copy of the object.
                    m_model.delete_object(m_model.objects.size() - 1);
void
TMFParserContext::apply_transformation(ModelObject *object, std::vector<double> &transformations)
{
    // Apply scale.
    Pointf3 vec(transformations[3], transformations[4], transformations[5]);
    object->scale(vec);

    // Apply x, y & z rotation.
    object->rotate(transformations[6], X);
    object->rotate(transformations[7], Y);
    object->rotate(transformations[8], Z);

    // Apply translation.
    object->translate(transformations[0], transformations[1], transformations[2]);
    return;
}
Samir55 commented 7 years ago

Dev Log(17- July):

Samir55 commented 7 years ago

Some Screenshots:

Those are 3mf models found in this repository. screenshot from 2017-07-18 04 16 17

screenshot from 2017-07-18 04 21 22

Samir55 commented 7 years ago

Dev Log (23- July):

Samir55 commented 7 years ago

Dev Log( 25- July):

Samir55 commented 7 years ago

@alexrj @lordofhyphens I need some overall code review. If there is something that needs modifications or improvements tell me.