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

Base type interfaces for soluble interface supports #6017

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

spiky2021
Copy link

At the moment soluble support material adhesion is weak due to sparse support layers under soluble support layers. I reported as issue #5823 with pictures, as well.
I modified two methods to the SupportMaterial Class including their headers.
The new methods add two base type interface layers to the support structure, in case the extruders are different and soluble support is choosen.
Since it is conditionally activated, it in general doesn't need a GUI input. But a GUI option number of base interface layers may enabled users to adapt this feature to their needs.
This is my second try to provide a pull request on this topic. Reset my fromer repository, because first I merged this and all other changes to my master and couldn't provide separate pull request anymore.

At the moment soluble support material adhesion is weak due to sparse support layers under soluble support layers. I reported as issue prusa3d#5823 with pictures, as well.
I modified two methods to the SupportMaterial Class including their headers.
The new methods add two base type interface layers to the support structure, in case the extruders are different and soluble support is choosen.
Since it is conditionally activated, it in general doesn't need a GUI input. But a GUI option number of base interface layers may enabled users to adapt this feature to their needs.
This is my second try to provide a pull request on this topic. Reset my fromer repository, because first I merged this and all other changes to my master and couldn't provide separate pull request anymore.
@bubnikv
Copy link
Collaborator

bubnikv commented Feb 11, 2021

thanks, I will review it tomorrow.

@bubnikv
Copy link
Collaborator

bubnikv commented Feb 11, 2021

Merged manually into spiky2021-sp_base_interfaces
fixed after merging.

I am running it, at the first shot it seems to be doing what it should. I am passing the build to our testers.

@spiky2021
Copy link
Author

My major concerns are not the basic functionallity. Its more, that for very small supports two base type layers may be too much. Further I am still worried, if it was a good idea to merge the base interfaces with the base during toolpath generation. It may lead to long travels without retraction, though it would just affect the support. Therefore I am still optimistic, that it achieves a good test result.

@spiky2021
Copy link
Author

Dear Vojtěch Bubník,

I checked your modifications and saw that you did some nice addons and clean ups.
But now I compiled, removed a few bugs related to the exchange of the object to
the layers reference in the headers, etc. and restored the old idx_higher_or_equal template,
because the new one doesn't compile so far.
Now by testing, I saw, that many base type interface layers are generated inside the soluble
ones, means the solution doesn't work robust. Maybe it is just related to a wrong indexing?
But my concern is, that it originates from normal and base type interfaces generated in
one step. Since this is done in parallel, it is really difficult by an algorithm to decide, when and
where which type should be generated.
I just can say, that I first tried it the same way, but always struggled with the parallelization during
different test. Therefore at the end I did it in seperated steps. Especially because such problems
are compiler and maschine dependend, as well.
Since in both cases the code runs in parallel, there is just the overhead of double calling and invoking
the parallelization.
My implementation was a little dirty, because I called the method twice in the generate method.
Whatever, if my worrie confirms, I suggest to go back to my former implemetation, protect it
and wrap its double call in a further method with a header like the old one, but returning both
vectors.

A remark, someone exchanged the object reference some headers of the methods by a support
layers reference. In general this makes it simpler, but if this is not done for all methods the code
becomes less readable. Further for some methods support_layers is of type MyLayersPtr.

Cheers
Mike

@spiky2021
Copy link
Author

I tested a little more and found, the problem just happens for 2 inerface layers. I checked the code, as well.
I believe now it is no parallelization problem, your goal is, that you always subtract soluble layers first, geat!
I will investigate this a little more. For one interfaces layer no base type layer is generate any more.
For two without staggering due to an object surface, it is only 1, because you account it in total.
Maybe the problem with 2 layers is related too that, I will check!

@spiky2021
Copy link
Author

Dear Vojtěch Bubník,
I found the problem. It results from generating base interface layers with no polygons by the insert_layer function.
Everything works fine, if you do it like this:
auto insert_layer = [&layer_storage, &layer_storage_mutex](MyLayer &intermediate_layer, Polygons &bottom, Polygons &&top, SupporLayerType type) {
assert(! bottom.empty() || ! top.empty());
// Merge top into bottom, unite them with a safety offset.
append(bottom, std::move(top));
//layer_new.polygons = union_(std::move(bottom), true);
bottom = union_(std::move(bottom), true);
Polygons base_intersection_polys = intersection(intermediate_layer.polygons, bottom);
if(!base_intersection_polys.empty()){
MyLayer &layer_new = layer_allocate(layer_storage, layer_storage_mutex, type);
layer_new.print_z = intermediate_layer.print_z;
layer_new.bottom_z = intermediate_layer.bottom_z;
layer_new.height = intermediate_layer.height;
layer_new.bridging = intermediate_layer.bridging;
layer_new.polygons = base_intersection_polys;
intermediate_layer.polygons = diff(intermediate_layer.polygons, layer_new.polygons, false);
return &layer_new;
}
return (MyLayer*) nullptr;
};
So far you didn't answer, if you don't again, I will make a pull request?!
Cheers

@bubnikv
Copy link
Collaborator

bubnikv commented Feb 18, 2021

@spiky2021 Thanks for your effort. I am tired today, I hope to look into it tomorrow. Would you please share a 3MF file and screenshots showing what is wrong?
For such a small change a pull request is not needed.

@spiky2021
Copy link
Author

Hi,
if I use the code included above a slicing of my testobject looks like this:
Sliced_without_empty_polygon_layers
If I use your original code with the small debugging described above it looks like:
Sliced_with_empty_polygon_layers
while always two interface layers set.
3MFs are:
3MF_files.zip
Further i found, that having just one base interface layer in some cases leads to a bottom side gap between the support base and soluble interfaces. This is true for all codings, even my old one (means not filler dependent), because the base interface polygon surface is to small and thrown out:
Sliced_without_empty_polygon_layers_missing bottom_contact
Well, of cause it vanishes with different support orientations (angle) for this simple object. But for more complicated objects this may be a problem? So far, I don't know, if in general one can live with this?

One question: The intention by my old coding was to allow a single solubel interface layers with adhesion support by a base type layer, as well. In cases where the object surface isn't parallel to the bed, like in my example, this can save alot of time due to head or filament changes. Your actual coding doesn't allow that, what is your intention to do it this way?

@spiky2021
Copy link
Author

I have to say sorry Vojtěch Bubník,
I jsut saw that for your coding the gap on bottom side doesn't exist, but therefore the mixing of materials in interface layers.
In reality the different behaviour is not related to empty layers, but to using a polygon diff or a polygon intersection to derive the bass type polygons. Unforntunately both ways lead to problems???

@spiky2021
Copy link
Author

Dear Vojtěch Bubník,
the last two days I tested alot of implementations using intersections or direct contact polygons and combinations for your
new approach as well as my pulled one. It showed, that using the polygons directly always leads to mixing of layers. By testing your implemntation I found, that the subtractions of interfaces layers from the base type ones didn't work for my compilation.

Therefore I implemented another approach like explained above. The generate method wrap in further one, having a header reference to before generated interface layers. With this the subtraction (no intersection) worked, but the result was the same as with the simple intermediate layer intersection. The bottom side gap between the two interfaces types for tilted surfaces apperead again and is there for your implementation for more the 3 layers, as well.

Thus I stop it and concentrated on removing the gap for the intersection approach. Unfortunately I only found a workaround by forcing always two base type bottoms layer, if at least two interface are requested. Well, this solves it only for
2 and 3 and further odd numbers requested. The attached picture shows it for 2 layers.
Sliced_two_bottom_base_interfaces

After all I suggest to go back the intersection approach like the coding above, which was already used by the original PS implementation.
If you like to test implication by forcing always two bottom base type layers, then simply extend your coding by the bold parts:
if (num_base_interface_layers > 0) {
// Some base interface layers will be generated.
if (num_interface_layers_only == 0)
// Only base interface layers to generate.
std::swap(top_inteface_z, bottom_interface_z);
if(num_base_interface_layers == 1) //If this layers, than always 2 for bottom to workaround an interface gap for surfaces with arbitrary angle.
bottom_z = intermediate_layers[std::max(0, idx_intermediate_layer - num_interface_layers)]->bottom_z;
else {
top_inteface_z = intermediate_layers[std::min(num_intermediate - 1, idx_intermediate_layer + num_interface_layers_only - 1)]->print_z;
if(num_base_interface_layers == 1) //If this layers, than always 2 for bottom to workaround an interface gap for surfaces with arbitrary angle.
bottom_interface_z = intermediate_layers[std::max(0, idx_intermediate_layer - num_interface_layers_only - 1)]->bottom_z;
else
bottom_interface_z = intermediate_layers[std::max(0, idx_intermediate_layer - num_interface_layers_only)]->bottom_z;

}
}

I decided to keep it like it is now and for the meantime use my own installer for my company. Unfortunately the actual support material code makes it really difficult to achieve simple things like this.

In my opinion it is worth to rework the thing from scratch. An implementation storing top and bottom contacts during bottom contact generation in colunm containers holding this two pointers and relate it to the support layer structure derived from the object or elsewhere. Something similar to intermediate layers. Anything between the contacts is only stored by a layer type specifier vector. In a second step touching columns are merge to super-columns. These further contain a reduced central polygon allowing a concave scolunm shape or a simliar approach. Further in certain cases the bottom half of a column is truncated resulting in tree like shapes of the scolumns. In a third step for each layer type all island polygons are generated and appended to layers. At the end maybe the object needs to be truncated again, to be save?
It would save a lot of computations (especially for all the searches) as well as memory and may achieves a higher accuracy due to less subtractions and intersections.
A big implementation work, but would make it smarter and easy extendable afterwards.
What do think, is it worth? Maybe I can invest some time to it?

Cheers
Mike

bubnikv added a commit that referenced this pull request Feb 25, 2021
Base type interfaces for soluble interface supports #6017
@bubnikv
Copy link
Collaborator

bubnikv commented Feb 25, 2021

Mike, I have fixed one regression (bug introduced by me, SupportMaterial.cpp:2735, missing intersection with the source base layer) and I have also fixed some missing clipping in the raft layers, see 750cfdd

Frankly I am having difficulty to understand the other issues you are having. Please document all your reasoning with pictures.

@bubnikv
Copy link
Collaborator

bubnikv commented Feb 25, 2021

I understand you worry that too much of the soluble material is being wasted.
More importantly, the non-soluble interfaces are often not being supported well when trimmed by the soluble interface.

image

The solution here is to not expand the soluble interfaces into the grid, but to just smoothen these islands. I hope to try that tomorrow, however I will need to implement the smoothing function or to borrow it from Cura ConstPolygonRef::smooth_outward(). I am afraid that without this extension the soluble interfaces / non soluble bases feature would be broken.

@spiky2021
Copy link
Author

Dear Vojtěch,
one problem is with the support structure outcome. As you can see in the picture supports build on tiltet surfaces in some cases have a gap between the interface and base interface. This is no problem for this sliced objects, since the support is expandet to the outside, somehow like tree supports. But there can be case where expasion doesn't work?!
The others are related to robustnest of your algorithmen. In my compiled version and settings with equal 0.4 print width for all types, the gap pronounces strongly. With 0.35 for support its much less. But a real problem is the mixing of both types of interface layers visible in above pictures. I just can say, yesterday I prepared an installer using your combined method but with above insert_layer function. It work well for my test object with different orientation and same print setting.
Then today I liked to finished a complete setup for this slicer with default print setting easily importable by our engineers. But tests with permutions of this settings and object orientations showed, that even for horizontal surfaces the basetype layer often disappeared. Thus I went back to the solution running the generate method twice wrapped in a further method. This time all tests with different layer heights, print widths and even variable layer heights worked fine. If I compare both approaches I can't find a difference, except possible parallelization impacts. Maybe the insert_layer function and/or the contact collections loops are broken to different tasks? If so, it would explain as well, that the pointer you give to the second call of the insert layer function for the base type layer hadn't lead to any subtraction for my compilations.
I hope that helps you? More picture than the above ones I don't have at the moment.
Cheers
Mike

@spiky2021
Copy link
Author

Sorry forgot tosay, yes i recognized the problem with contact sizes that not really nice and somtimes may hurt, but i though in most casrs that can be avoided by different support angles??

@spiky2021
Copy link
Author

I forgot to say! For testing why subtraction of interface polygons from base interface polygons in your implementation didn't work, I disabled the parallelization yesterday, but with no change. Therefore I trusted your implemention und build the installer. Means I'm unknown about the reason and to busy to analyse it.

@bubnikv
Copy link
Collaborator

bubnikv commented Feb 26, 2021

Then today I liked to finished a complete setup for this slicer with default print setting easily importable by our engineers. But tests with permutions of this settings and object orientations showed, that even for horizontal surfaces the basetype layer often disappeared.

Please provide test cases where the current master fails, possibly together with the pictures. Thank you.

@spiky2021
Copy link
Author

Dear Vojtěch,
to the problem of lousy supported base interface layers I saw that it doesn't exist for supports with shell, since there base polygons are reduced by one support print width.
BaseInterfacesWithShell
A problem with lousy supported interfaces occures for the actual PS release as well, only less often. Means it is a problem of interfaces in general.
InterfacesActualRelease
My suggestion is not to look at the interfaces and expand them to the outside, since this may cause other problems e.g. for supports with shell, etc. Better reduce the base polygons like for supports with shell in case base interfaces exist, or in general, if one encounters my second picture??
If nothing works and you, as well as other responsibles for PS don't want to live with it, the last already working solution is to active base interfaces only in case shells are requested.
Cheers
Mike

@spiky2021
Copy link
Author

Hi again,
sorry hadn't seen, that you already answered.
With current master I believe you mean your most actual implementation. I have to downlaod it first and check it!
Cheers

@bubnikv
Copy link
Collaborator

bubnikv commented Feb 26, 2021

When you take such screenshots, please provide a 3MF. Also provide a screenshot of the whole scene, so I can see the spot you zoomed into. Thank you.

@bubnikv
Copy link
Collaborator

bubnikv commented Feb 26, 2021

to the problem of lousy supported base interface layers I saw that it doesn't exist for supports with shell, since there base polygons are reduced by one support print width.

I am sorry, but I don't understand. The more information you can provide, possibly with pictures, the better your information will be understood.

What is a support with shell?

@spiky2021
Copy link
Author

The 3MF files.
3MF_lousy_interface_base.zip
Not exactly same settings, since the release version I tested with a still install 2.30 alpha 4 version.
Support with shell was short for I activated the outer shells for supports.
I have a german language version, thus I don't know how it is called in the english one.

@spiky2021
Copy link
Author

Maybe it helps if send you my actual imlentation as .cpp .hpp .
latestWithDifferentAproaches.zip
In .cpp I added complier flags, thus you can easily change between implementations.

@spiky2021
Copy link
Author

Oh, forgot to say. It is not based on your actual implementation so far.

@spiky2021
Copy link
Author

Ok downloaded your actual implementation .cpp and .hpp.
But have problems for the generate_raft_base method, some brim stuff is missing like object.has_brim(), etc.
Is this added by you or is my PS version laready outdated at that point?

@spiky2021
Copy link
Author

Tried to fix it with patching a few files. But dependencies are to many. Have to download full sources.

@spiky2021
Copy link
Author

Dear Vojtěch,
I started to build a further PS master, but the PC I am building on stoped me. Its Disk is partioned as C: 120Gb (15 Gb free) and D: 1,5Tb. Now VS2019 does not allow me to build on D: and I don't want to overwrite my PS 2.3.0 build at the moment.
Stupid situation!

@spiky2021
Copy link
Author

Dear Vojtěch,
set a junction to drive D: , since C: is a SSD with above size. Compiling master now and then will check your improvements.
Cheers

@spiky2021
Copy link
Author

Dear Vojtěch,
compilation works now. Your implementation as well. It is now similar to mine, but your implementation crashes as
soon I activate outer shells around support.
I attached 3MF, all settings and a picture how it looks like without activating the
shells in a zip.
CrashWithActualMasterPS.zip
If you use the same settings as you can see in the picture and activate the shells, you can provoke the crash.
Cheers Mike

@spiky2021
Copy link
Author

Just a moment ago I came to the idea to choose setting "0.15 IDEX" instead of "0.15 IDEX soluble" and activated supports everywhere.
It works OK. But if I activate the outer shell around supports, too, it crashes as well. Seems like there is a problem with the base support happening??
Cheers

@bubnikv
Copy link
Collaborator

bubnikv commented Mar 3, 2021 via email

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

2 participants