FullControlXYZ / fullcontrol

Python version of FullControl for toolpath design (and more) - the readme below is best source of information
GNU General Public License v3.0
672 stars 78 forks source link

Initial mesh export work #25

Closed ES-Alexander closed 9 months ago

ES-Alexander commented 1 year ago

Relevant to #20

fullcontrol-xyz commented 1 year ago

Great to see this! Let me know if there are any specific things I can help with. For me (Andy), your code is interesting to see, and teaching me a lot, but I'm probably not adept enough (yet!) to be able to do any significant development on it. But for any questions about implementation concepts, etc., let me know. At the moment, I can see this as being an STL model of the plot, which makes sense. But if one of the main aims is to lead into an FEA demonstration, I'd suggest replacing the oval geometry concept with a rectangle so that the top and bottom of tube have a nice flat contact region. Even with just two long triangles per face, pymeshlab will no doubt have filters to subdivide them. Then a simple rectilinear lattice could be generated quickly and used in FEA quickly and a demonstrator workflow. I have used FEA software plenty so can do some tests, but I haven't used python-based FEA software, which I know exists and would be amazing to incorporate. I'll ask around my research group to see if anyone has that experience.

Alternatively, if you're just focusing on getting a good visual model out for the time being, this FEA stuff doesn't matter yet.

ES-Alexander commented 1 year ago

Rectangular cross-section should be easy to add as an option or subclass, and I am planning to do it - I just forgot to put it on the list (but have added it now) :-)

It’d be good to do at least some simple tests before it gets merged, but it’s not ready for that yet. At this stage I’m not even sure if the current STL generation code works because I haven’t had a chance to run it yet - I just wrote some code that seemed to follow the STL specs in Wikipedia.

I’m pretty flat out with work at the moment, but I’ll work on this when I’m able to - should hopefully have some time on the weekend at least :-)

fullcontrol-xyz commented 1 year ago

Great yep that's all fine. I'll definitely do some testing. If this first version is going to be conceptually an extension of the plot function, perhaps tube_type could have options of flow | cylinders | cuboids and then perhaps an export attribute with stl_ascii | stl_binary initially. This is just my first thought. I'm aware that as things grow, some more formal structure will be required to avoid massive parameters lists being required and their option strings memorized or frequently looked up by users. So the above suggestion only adds one more attribute. But I may be overthinking things. Anyway, we can see how things 'feel' when you've got the preliminary verison working

ES-Alexander commented 1 year ago

Added a straightforward rectangular cross-section option, then had a bit of fun adding a roundedness parameter to the elliptical cross-section type so it can be turned into a super-ellipse, which is effectively a rectangle with specifiable roundedness (which may be good for more realistic renders and/or simulating layer lines in stacked sections - presumably a circular cross-section makes more sense for freestanding bridge/lattice lines and the like): Screenshot 2023-06-22 at 4 14 25 pmScreenshot 2023-06-22 at 4 08 54 pm

AndyGlx commented 1 year ago

This is really cool. It could definitely be used for real design work. I guess ideally with the profile being defined by the designer in an enhanced version of the current ExtrusionGeometry. I can see the ellipse being valuable for many things.

A similar geometry that would be v useful for FEA, to avoid point contacts, would be a direct chamfered or rounded rectangle, since it would have flat areas of contact between extrudes on different layers or side-to-side. Perhaps with user-defined width-fraction and height-fraction of the top/bottom and left/right flat sections.

These kind of things, and the differences you mentioned between different geometric situations are exactly the sort of thing FullControl is intended to encourage design of. In many structures you have combinations. Like tissue engineering scaffolds with some solid and some entirely lattice sections. Or shell vs infill for some print paths.

How about we add ellipse (and super-ellipse?) to ExtrusionGeometry(), along with a more controllable rounded/chamfered rectangle? The necessary information for tube generation can be supplied in plot_data. It might make sense for the point generation algorithms (points around the tube) to be methods in that object. That could make it easy for people to add their own profile concepts, by editing a single file. All just spitballing for now.

aapolipponen commented 1 year ago

I haven't followed the mesh development that closely. Are the tubes that create the shape being exported? I myself am looking for a solution that creates a "smooth" mesh for the wing project, I've been working on. Is that going to be implemented at some point or will I do it myself? Constructing it would work kind of using the points, not the lines between them.

ES-Alexander commented 1 year ago

A similar geometry that would be v useful for FEA, to avoid point contacts, would be a direct chamfered or rounded rectangle, since it would have flat areas of contact between extrudes on different layers or side-to-side.

@AndyGlx I've just had a potentially bright idea for how to achieve that mostly within the existing structure - see the new flat_sides parameter description. ~For reference, with sides=8 (i.e. a chamfered rectangle) and rounding_strength=0.2 I got a dimensional reduction of \~1.57% from the specified width and height, and 0.79% with rounding_strength=0.1 (although the chamfer in that case is rather shallow).~

~Is that kind of tradeoff acceptable, or would there be significant value in making a separate chamfered cross-section implementation that exactly matches the specified width and height? I'm wary of making it unwieldy by adding too many parameters and options to choose between (both from a usage and maintenance perspective), but if necessary we could (relatively easily) shoehorn that kind of thing into the code as a special case of the existing parameters (e.g. if sides == 8 and flat_sides: ..., and treat rounding_strength as an exact chamfer height or depth or something).~

EDIT: I realised the "error" could just be corrected for when it's appropriate to do so, since the rounding effect will only ever shrink the cross-section, so we can expand it to compensate when the sides are flat (so they're now dimensionally accurate to the specified width/height) :-)

It might make sense for the point generation algorithms (points around the tube) to be methods in that object. That could make it easy for people to add their own profile concepts, by editing a single file.

If custom cross-section profiles are an important/desirable possibility then that can be achieved by overriding the calculate_point_offsets method (which I've just split out so that that's possible). From a user interface perspective we could allow a user to just provide a function that can do the relevant calculation and we can monkey-patch it in for them before the tube generation occurs. Once the main functionality of this PR is sorted out I can work on making that nice if you want, but otherwise it can happen in a later PR.


I haven't followed the mesh development that closely. Are the tubes that create the shape being exported?

@aapolipponen yes, the idea with the mesh generation is for the user to specify an extrusion trajectory, and we generate a mesh of the extruded filament that shows what the print would look like in a physical volume, and this PR is about making it possible to export that mesh (along with adding some quality of life improvements for people who want to do interesting things with the mesh afterwards).

I myself am looking for a solution that creates a "smooth" mesh for the wing project, I've been working on. Is that going to be implemented at some point or will I do it myself? Constructing it would work kind of using the points, not the lines between them.

I'm not sure what you mean by this - if you're looking to create a normal mesh of an object from some form of point-cloud then that's seemingly outside the scope of the FullControl project (since it's entirely unrelated to gcode and the machine operations relevant to creating said object), and there are several other programs and libraries that can help you do that.

If instead you've generated a toolpath for a 3D printer to create a wing then yes, this PR should make it possible to export a mesh of that, although as mentioned the mesh will be of the predicted extrusions, not a higher level general mesh of the main object geometry. That said, it may be possible to process an extrusion mesh that we generate into a more general mesh of the object, using something like MeshLab.

aapolipponen commented 1 year ago

@ES-Alexander Yea, my description was pretty unclear. You still kind of got it. The mesh would be created from the point-cloud of the fc.Points. So if I have for example generated the toolpath to create a perimeter of an wing for example, the fc.Points will be used to generate the mesh. So I am looking to create a normal mesh. I'm actually not sure if I understand the process of creating a mesh correctly, because I haven't looked into it but that's basically the thing I'm imagining. I will implement that in my wing project.

ES-Alexander commented 1 year ago

The mesh would be created from the point-cloud of the fc.Points. So if I have for example generated the toolpath to create a perimeter of an wing for example, the fc.Points will be used to generate the mesh. So I am looking to create a normal mesh. I'm actually not sure if I understand the process of creating a mesh correctly, because I haven't looked into it but that's basically the thing I'm imagining.

Fair enough. There'll likely be several resources available online if you want to work on that - try searching for things like "Python mesh from point cloud" and "Python surface from point cloud".

Otherwise if you think it's relevant to FullControl you could raise an Issue to start some relevant discussion and see whether anyone's interested in helping to develop the feature :-)

In case it's relevant/of interest, FullControl is already using the Plotly library for visualising the extruder mesh, and Plotly has some built in points -> mesh functionality (which you may wish to try out).

ES-Alexander commented 1 year ago

Small update to change the triangle point ordering to be consistent (which is important for mesh files, and fixes the visualisation shading to also be consistent, especially at the endcaps).

New screenshot (right) shows the improved shadows and the new flat_sides functionality - both were made with sides=12 and rounding_strength=0.5: Screenshot 2023-06-22 at 4 08 54 pm --> Screenshot 2023-06-23 at 6 46 22 pm

ES-Alexander commented 1 year ago

Another small edit, to make the cross section expand to the full width/height when flat_sides is used.

fullcontrol-xyz commented 1 year ago

@AndyGlx I've just had a potentially bright idea for how to achieve that mostly within the existing structure - see the new flat_sides parameter description. EDIT: I realised the "error" could just be corrected for when it's appropriate to do so, since the rounding effect will only ever shrink the cross-section, so we can expand it to compensate when the sides are flat (so they're now dimensionally accurate to the specified width/height) :-)

It might make sense for the point generation algorithms (points around the tube) to be methods in that object. That could make it easy for people to add their own profile concepts, by editing a single file.

If custom cross-section profiles are an important/desirable possibility then that can be achieved by overriding the calculate_point_offsets method (which I've just split out so that that's possible). From a user interface perspective we could allow a user to just provide a function that can do the relevant calculation and we can monkey-patch it in for them before the tube generation occurs. Once the main functionality of this PR is sorted out I can work on making that nice if you want, but otherwise it can happen in a later PR.

This is great! These tubes are looking really appropriate for FEA now. I hope this will really have an impact for researchers immediately and then industry as well once the FEA stage is automated. Open-source structure+toolpath optimisation in a single python notebook will just be amazing!

I think the current approach with flat sides and rounding works well. There's an interesting quirk that using non-flat sides and tube_sides = 6, 10, 14, etc., results in flat top/bottoms. It feels like there may be an alternative, but very similar, set of parameters that might be more clear to designers. But I can't think of anything now. The current ones are the clearest I can think of. For flat_sides=False, you tend towards a diamond as you get more coarse and for flat_sides=True, you tend towards a rectangle as your get more coarse. And for lots of tube_sides, they both converge on the same result. I'll keep thinking, but I'm really liking it as it is!

I had previously noticed that for tube_sides = 6, the tubes were not the 'correct' height because of the ellipse approach and no points being directly on the minor axis. It wasn't a major issue for visualisation only, but it's nice that your update to make rectangles work has also fixed that!

I think flat_side=True and tube_sides=8 is a good default. Or flat_side=False and tube_sides=6 Potentially with rounding_strength=0.5

ES-Alexander commented 1 year ago

These tubes are looking really appropriate for FEA now.

Still gotta sort out the corners, but the cross-section options do seem more likely to work in a desirable manner :-)

I hope this will really have an impact for researchers immediately and then industry as well once the FEA stage is automated. Open-source structure+toolpath optimisation in a single python notebook will just be amazing!

Agreed - part of why I'm interested in this project is it seems like it has the potential to be very helpful to a bunch of people doing interesting things, which makes it extra fun to contribute to :-)

... for lots of tube_sides, they both converge on the same result.

That's to be expected - the rounding effect (which in reality is more of a "squaring" of the initial ellipse) is applied the same way, the only difference is whether the first point of the cross-section lies on the major axis line (in which case it's impossible to have a flat side there) or half a side's rotation above it (in which case side numbers with symmetry guarantee flat sides). With many sides that difference becomes minimal. To make the shape more rectangular when there are many points in the cross-section (many sides) just requires a low degree of rounding (e.g. rounding_strength=0.1 is very rectangular).

I had previously noticed that for tube_sides = 6, the tubes were not the 'correct' height because of the ellipse approach and no points being directly on the minor axis. It wasn't a major issue for visualisation only, but it's nice that your update to make rectangles work has also fixed that!

Not certain what you mean by this - the non-full height will still be the case if flat_sides=False (because it's just sampling an ellipse), but it will be expanded to the full width now when flat_sides=True. It should be possible to expand to full height when flat_sides=False and there are an even number of sides (since there'll be top/bottom symmetry) if that's desirable though?

fullcontrol-xyz commented 1 year ago

Ah yes, I thought it was expanded to full height but it's still not. The z value of the point being probed in this image should be z=0 in real life: image

Since the designer is explicitly specifying extrusion width and height, it does seem most logical to expand the tube to match those values. So, I think expanding to full height is good.

One alternative to flat_sides would be to give the designer options for tube_shape = 'ellipse' | 'rectangle' | 'chamfered_rectangle'. Some of these might not utilise tube_sides (e.g. they automatically set it to be 4 or 8). And they may change the default value of rounding_strength. In the future, more shapes could be introduced like 'rounded_rectangle', but not right now. Also in the future, the shapes could be taken from the design as we previously mentioned. And it's a step towards the custom profile concept (quote below), which I think this will be a fascinating addition, but it's not necessary now - I think a later PR is most appropriate for that.

Previous comment:

If custom cross-section profiles are an important/desirable possibility then that can be achieved by overriding the calculate_point_offsets method (which I've just split out so that that's possible). From a user interface perspective we could allow a user to just provide a function that can do the relevant calculation and we can monkey-patch it in for them before the tube generation occurs. Once the main functionality of this PR is sorted out I can work on making that nice if you want, but otherwise it can happen in a later PR.

ES-Alexander commented 1 year ago

Since the designer is explicitly specifying extrusion width and height, it does seem most logical to expand the tube to match those values. So, I think expanding to full height is good.

Fair enough. Done. I also realised I was calculating the correction factor incorrectly, which I've now fixed as well.

One alternative to flat_sides would be to give the designer options for tube_shape = 'ellipse' | 'rectangle' | 'chamfered_rectangle'. ...

A fixed enumeration like that might make sense for the higher level user interface (e.g. in the plot settings and the like), but I think it's better to leave the class initialisation options reasonably transparent as to what it's actually doing (and what it's capable of), so it's easier for others to understand and maintain the code in future. Thoughts?

AndyGlx commented 1 year ago

Yep, completely agree. Good to separate the function from what designers choose in PlotControls. Keeps things simpler all round

ES-Alexander commented 1 year ago

FYI I'll be travelling for the next few weeks and am unsure whether I'll be able to work on this during that period.

I'm hopeful I'll be able to make some time for the corner stuff soonish, but if someone wants to test something in the meantime I think the FlowTubeMesh STLs should be ok as-is for paths that have no angles <= 90° between successive path segments (e.g. a helical spiral may be a good test, or a path with multiple points used to round out out any sharp corners).

AndyGlx commented 1 year ago

Ah perfect. To output an stl, I'd just replace the current line in plotly.py?

replace: fig.add_trace(Mesh(path_points, widths=widths, heights=heights, sides=sides, capped=capped, inplace_path=True).to_Mesh3d(colors=colors_now))

with: Mesh(path_points, widths=widths, heights=heights, sides=sides, capped=capped, inplace_path=True).to_stl('myfile.stl')

ES-Alexander commented 1 year ago

To output an stl, I'd just replace the current line in plotly.py?

replace: fig.add_trace(Mesh(path_points, widths=widths, heights=heights, sides=sides, capped=capped, inplace_path=True).to_Mesh3d(colors=colors_now))

with: Mesh(path_points, widths=widths, heights=heights, sides=sides, capped=capped, inplace_path=True).to_stl('myfile.stl')

That should work, but if you have multiple extrusions then that would only save the last one (since you specified a constant filename).

You could slightly alleviate that by doing something like

for index, path in enumerate(data.paths):
    ...
    mesh = Mesh(...)
    fig.add_trace(mesh.to_Mesh3d(...))
    mesh.to_stl(f'myfile{index}.stl') # save STL

but that would still have overwriting issues if you "plot"ted multiple without moving / renaming the STLs in between.

I suppose you could just also add a timestamp to the filenames to avoid them overwriting each other, but that might not help much with knowing which file is which without actually opening / previewing the files. If you want to do that the easiest way is likely

from datetime import datetime

...
for index, path in enumerate(data.paths):
    ...
    mesh.to_stl(f'output_{index}_{datetime.today()}.stl')

For more meaningful filenames it's probably easiest to just add a plot control for setting the base, and then formatting that if you want indices and/or timestamps included (e.g. something like PlotControls(filename='fancy_helix_{}_{}.stl') followed by mesh.to_stl(controls.filename.format(index, datetime.today()))).

AndyGlx commented 1 year ago

Brilliant, thanks for these tips. I'll test it tomorrow

fullcontrol-xyz commented 1 year ago

I just tried and got the few errors...

The following line threw an error that 'FlowTubeMesh' object has no attribute '_mesh_normals' Similar for _triangle_points

If I comment out the following lines:

if (cached := getattr(self, '_triangle_points')) is not None:
    return cached

and

if (cached := getattr(self, '_mesh_normals')) is not None:
    return cached

Then I get an error ('FlowTubeMesh' object has no attribute 'triangle_indices') for the following line:

self._triangle_points = self.mesh_points[self.triangle_indices.flatten()]

any suggestions for fixes?

These all happened when I changed plotly.py to be

mesh = Mesh(path_points, widths=widths, heights=heights, sides=sides, capped=capped, inplace_path=True)
fig.add_trace(mesh.to_Mesh3d(colors=colors_now))
mesh.to_STL(f'myfile.stl')  # save STL

and test with:

import fullcontrol as fc
steps = fc.rectangleXY(fc.Point(x=0, y=0, z=0), 20, 15)
fc.transform(steps, 'plot')
ES-Alexander commented 1 year ago

I just tried and got the few errors...

@fullcontrol-xyz Sorry about that - I hadn't tested so there were a couple of minor issues and typos. I've fixed those issues now and actually tried generating a couple of STLs as examples, and they now seem to work as I'd expected :-)

I also added an "overwrite" argument to to_STL, so it appends the current datetime to the specified filename if the file already exists and the user has not explicitly requested to overwrite it.

Here's the result of visualising the example ones in PrusaSlicer - both ASCII and binary outputs seem to be working:

Screenshot 2023-07-01 at 10 09 10 pm
AndyGlx commented 1 year ago

Great thanks, I'll play around. With regards to corners sharper than 90 degrees, I can see you've got one there. So it's it the case that they work okay, but just don't have nice corner geometry yet?

fullcontrol-xyz commented 1 year ago

Looking good after playing for a while.

I noticed that there is undersizing (layers weren't tall enough) when tube_sides are not manually set by the user. The error disappeared when I gave tube_sides a default value of 6 instead of None in fullcontrol.visualize.controls.py. I was confused for a while as to why my editing the default value in init() has have no effect or confusing effects. What do you think about removing the default values in __init__() in tube_mesh.py and forcing them to be specified when creating the object. Or perhaps do this remove the default values for parameters that are options for the end user in fullcontrol. It seems like it'd be best to only default them once in the codebase, but I'm not sure of exact best practice.

I'll keep playing around with some more complex geometry

If you do any further mods, and agree with these suggestions, can you implement them:

I'm not that familiar with push-pulling and don't wanna mess up your code... if I make some changes can I push them to this pull request? e.g.

git checkout -b ES-Alexander-mesh-export master
git pull https://github.com/ES-Alexander/fullcontrol.git mesh-export

make some changes

git add .
git commit -m 'message message message'
git push 

do you know if I need to add any extra commands about the branch. git checkout master or something before doing my commit and push?

Thanks!

ES-Alexander commented 1 year ago

If you make a pull request to my branch (in my fork) then I can merge in changes.

When I made this PR I did enable commits by the project maintainers, so you might be able to commit directly, but I’ve never actually needed to do that so don’t know what’s involved.

fullcontrol-xyz commented 1 year ago

Looking good with this demo! It's 10000 line segments with tube_sides=8.

image

And this is after using meshlab to create a surface mesh for the whole object instead of overlapping tubes (similar number of triangles in total) (Filters > Remeshing, simplication and reconstruction > Marching cubes (APSS) > default settings except MLS-filter-scale = 4 Plus closing holes with Filters > Remeshing, simplication and reconstruction >Close holes (default settings) image

This is brilliant!

ES-Alexander commented 1 year ago

Minor update to fix lowercase name and file timestamp format suggestions, and add 'cut' transitions as an option (but still with user-specified amounts, rather than a "minimal" approach that's determined automatically, and not yet adding any extra points).

ES-Alexander commented 1 year ago

I tried running Plotly's alphahull implementation on the points, with alpha=20, in an effort to compare alphahull with your meshlab approach. It took several minutes to generate, but the result seems pretty decent (although it's a bit hard to tell how much of the niceness is from the smooth rendering, and there doesn't seem to be an obvious way to export the data of the resulting mesh).

I'm curious whether we might be able to make our own implementation (e.g. compiled using Numba or similar) that makes use of knowledge of the extrusion path+mesh to significantly reduce the search space, effectively just doing something like a morphological closing on the existing volume.

Screenshot 2023-07-08 at 11 13 11 pm

Anyway, that's outside the scope of this PR, but some interesting food for thought...

fullcontrol-xyz commented 1 year ago

Ah cool, I didn't know plotly could do that. Yeh I'm sure there would be quite a few ways to implement our own custom strategies. Although meshlab is available in python (pymeshlab) and had quite a lot of other filters for meshing and repairing that might come in handy anyway (e.g. to close holes). I'll continue to play. Am away for two weeks now though so probably no major things until early Aug.

fullcontrol-xyz commented 1 year ago

FYI I'm back from various travels now and ready to continue this. We're pretty far along the list in your first comment on this page. What are your thoughts on functionality to include before implementing in FullControl. As we did with the tube visualisation, it'll be good to spend a few days and some hands on 'feeling' of what defaults are best, what parameters to offer to end users, what parameter names to use for end users, etc.

ES-Alexander commented 1 year ago

What are your thoughts on functionality to include before implementing in FullControl.

The only actual requirement is adding the feature to the FullControl API/settings, but I’d like to do something with the sharp corner handling if possible, because it seems valuable and will likely be harder to motivate once the initial functionality is merged. I’ll see if I can sort something out this weekend.

I think it’s fine to leave the fancy/advanced exporting for a later PR, and the intersection checking isn’t a big deal so whether it gets done here will likely depend on how fun or annoying it feels to implement.

fullcontrol-xyz commented 1 year ago

Okay, sounding good. Let me know if there's any input you need from me for that.

One note... multiple paths in a single design should be outputted as a single stl containing multiple solid bodies rather than lots of separate stls. I can't remember if that's already the case, but don't want to forget about it so am writing this comment.

ES-Alexander commented 1 year ago

… multiple paths in a single design should be outputted as a single stl containing multiple solid bodies rather than lots of separate stls. I can't remember if that's already the case …

That’s not currently the case (because at the moment the meshes are generated from the path objects), and (at least as I understand it) that’s generally not recommended because the STL file format doesn’t have intentional support for multiple bodies (i.e. there’s no body differentiator field or element, so it effectively assumes there’s only one body specified by a given mesh).

Multi-body support in a single file should ideally be handled in a different file format that’s intended for that functionality, but it’s at least easy to implement if we want to do it with an STL file anyway (e.g. if you’ve got a simulation software in mind that’s ok with handling a single mesh that has multiple objects in it).

As something of a compromise, if we implement the tube intersections by proximity check then it should be reasonably straightforward to use that in determining connected components, so we could potentially export an STL per connected body instead of per extrusion path (since retractions and travel moves could commonly be used while creating a single object). Not sure how much effort would need to go into adding triangles to join the separate paths and form a closed object if we need a manifold mesh, although that could also be done with a boolean union in meshlab or similar if we don’t want to deal with/implement it ourselves.

Curious on your thoughts, but given the implementation simplicity I don’t see a problem with offering users a choice - it’s mainly a question of how to present that in an intuitive way that doesn’t result in accidentally creating files that may break in some softwares.

ES-Alexander commented 1 year ago

I’ll see if I can sort something out this weekend.

Unfortunately ran out of time for this, and I'm trying to get enough sleep this week so progress from me will likely have to wait until next weekend.

fullcontrol-xyz commented 1 year ago

Ah I didn't realise STLs were not supposed to have more than one solid. I've occasionally created stls with multiple bodies and they've worked fine with meshlab (my go-to meshing software) but I'm not sure how widely they're accepted. I think the best option is to go for simplicity and offer the user the flexibility to output multiple stls if they run into problems. So the default, if multiple paths exist, can be multiple bodies in one stl with a message printed to explain the stl has multiple bodies and that an optional parameter can be toggled to outputs the bodies and individual stl files, with a number suffix, if problems are encountered (ideally formatted '_01', '_02' if paths >= 10 and '_001', '_002' if paths >= 100, etc.).

Merging of multiple bodies is an interesting thing, but I think it's probably a common operator available in subsequent steps in the user's workflow (either with pymeshlab or similar in python, or using a desktop application like meshlab or CAD/FEA packages)

ES-Alexander commented 1 year ago

... the STL file format doesn’t have intentional support for multiple bodies (i.e. there’s no body differentiator field or element, so it effectively assumes there’s only one body specified by a given mesh)

Following up on this, apparently the ASCII STL format does have a way of explicitly specifying separate bodies, although it's unclear from the specification references I could find whether doing so is actually allowed / expected to be supported. The binary STL format doesn't have a recognised way of specifying bodies, but there is a two-byte field per facet that doesn't have a defined meaning, which some softwares may use to represent the body index (I know for sure that that field is used by some other softwares to represent facet colours and other properties, with multiple conflicting formats).

I've implemented multi-body inclusion as an option, and have added a warning message about software compatibility (as discussed).

On the software support side of things,

fullcontrol-xyz commented 1 year ago

This is looking good. I think there's enough support of multibody STLs for a warning message about software support to be an appropriate mitigation measure.

As long as the code is clear, the exact structure in the file is hopefully quite easy for someone to modify in the future. The real valuable stuff is all the geometry things you've done.

Although saying that, for non-codey people, having an easy stl output will be most valuable and likely widely used

ES-Alexander commented 1 year ago

I've added a quite minimal option for saving STLs when plotting tubes (since extrusion meshes are already being generated then anyway, so it saves needing to also generate them independently for saving).

I included PlotControls options to specify a filename, whether the STL(s) should be binary or ascii, and whether the output should be a combined file (or saved with each extrusion in its own file).

The STL exporting functionality is also capable of handling whether it should overwrite files with the specified name, as well as some extra metadata (the file units and author) for binary files, but those are currently not exposed in the plot controls because I thought doing so was excessive.

Those decisions are open to discussion, of course :-)

It's likely worth adding an example to the notebooks of STL saving when plotting, and we may also want to add a more detailed / advanced example of independent STL saving (without plotting) that can include the additional functionalities of the fullcontrol.visualize.tube_mesh.MeshExporter class. Not sure if it's better for me or @fullcontrol-xyz to create those examples, and my computer seems to be having issues with Jupyter at the moment, so I haven't attempted them yet.

ES-Alexander commented 1 year ago

I've marked this PR as ready for review, since I've been struggling to make time for this recently and don't want to hold things up more if the existing functionalities would already be useful.

I'm still hopeful I'll be able to implement an improved corner approach and the other uncompleted points from the PR description, but I can't guarantee a well-bounded completion interval for those at the moment, so it may make more sense to leave those for later PRs, assuming they're not seen as critical features in this one.

I will at least try to be responsive on any feedback relevant to getting this PR merged :-)

ES-Alexander commented 1 year ago

@fullcontrol-xyz is there something holding this up from getting merged?

I'm assuming you just haven't had a chance to review it yet, but figured I'd ping in case you weren't aware it's waiting for review :-)

fullcontrol-xyz commented 1 year ago

Hi @ES-Alexander, no sorry it's been on my to do list! Part of the issue was that I had implemented quite a few changes to fullcontrol that weren't quite complete and didn't want them and this to have any conflicts. I have now pushed them and am not currently working on any other changes. I also want to go through a few demo structures and figure out where the current defaults (tube_sides, rounding, flat_sides, etc.) are good for stl output. I'll get to it in the next week, maybe with a few questions/discussions on here.

fullcontrol-xyz commented 1 year ago

RIGHT! I'm on it. Time to get this in. So sorry for the delay. I've been thinking about how to get this easy to use and have the following, relatively minor, suggestions/questions.

How about we add a 3rd transform option? So we have gcode, plot and 3d_model (name TBC). In the actual code (fullcontrol.combinations.gcode_and_visualize.common.py -> transform()), the 3d_model transformation can actually use the plot function, just as you have currently written it, but the end user will not know or care about that. This means we can create a ModelControls object (name TBC). It will have all the essential PlotControls attributes, named identically, and can therefore be fed to the plot function you've currently written (maybe with a couple of minor code tweaks). This means we can add more attributes to the ModelControls object without worrying about cluttering up the PlotControls object. Good for the future, but also now because I'd like to take advantage by offering the user a couple of additional options. One being to have an option for something like rectangle_section, which if set to True, will automatically set the values of sides, rounding_strength and flat_sides without the end user needing to know about them. Going forwards, it might also include options to unionise the individual extrusions (using pymeshlab or something if need be).

I don't think this will change much of your code at all, but will hopefully make things clearer for end users who aren't keen to delve into the code.

I'm not sure, but it might be nice to put the third transform option into lab so the user writes fclab.transform() and this means we can modify it is future more freely since people are warned that lab stuff may change.

I'll also create a tutorial which shows the stl trnasformation. That will include a demo, or at least instructions, of how to use meshlab or pymeshlab to achieve a single union structure, so that the extrudates are all merged together (likely preferable for most users). This is another reason why I quite like the idea of creating a new controls object called ModelControls or something, because it means the tutorials can be more logically separated.

I'm happy to do the coding to implement those changes if you like. Any thoughts of my above comments? And on the names 3d_model, ModelControls and rectangle_section?

Thanks again for your effort and patience!

ES-Alexander commented 1 year ago

no sorry it's been on my to do list!

No worries :-)

Part of the issue was that I had implemented quite a few changes to fullcontrol that weren't quite complete and didn't want them and this to have any conflicts. I have now pushed them and am not currently working on any other changes.

Fair enough. I've just rebased this branch over master, so those changes should be incorporated now :-)

How about we add a 3rd transform option? So we have gcode, plot and 3d_model (name TBC).

That seems reasonable to me, although I haven't really been working with that part of the codebase so I'm not certain where the corresponding files would make the most sense - if you're willing to do that part then I'd probably prefer that :-)

This means we can create a ModelControls object (name TBC). It will have all the essential PlotControls attributes, named identically, and can therefore be fed to the plot function you've currently written (maybe with a couple of minor code tweaks). This means we can add more attributes to the ModelControls object without worrying about cluttering up the PlotControls object.

This seems both reasonable and valuable. Given there'll be a decent amount of overlap I'd recommend having either ModelControls inherit from PlotControls (and just override some of the values with more appropriate defaults, like style = 'tube'), or create a lower level BaseControls class that both inherit from, just to avoid the redundancy of defining each parameter in multiple places (and the extra maintenance that comes from doing so).

... have an option for something like rectangle_section, which if set to True, will automatically set the values of sides, rounding_strength and flat_sides without the end user needing to know about them.

This also makes sense. For scalability it should likely be handled within the relevant controls class, before it gets passed in to some kind of model creation or plotting stage. With a dataclass this could be quite nicely accomplished with the __post_init__ method, which runs after instance initialisation. I'm not sure whether the Pydantic BaseModel has something similar, or if it would just require overwriting the __init__ method.

I'm happy to do the coding to implement those changes if you like.

I'd appreciate that - it will likely take me some time to get up to speed on the relevant parts of the codebase otherwise.

Any thoughts ... on the names 3d_model, ModelControls and rectangle_section?

They seem fine to me :-) 3d_model could be a bit easier to type and remember if it's just model, but the extra clarity and beginner-friendliness from spelling it out is likely worth leaving it with the 3d_ in front.

fullcontrol-xyz commented 11 months ago

Sorry again this has taken so long! I've been meaning to get to this, and just kept getting drawn in to other things. Partly cos this is a little new to me... I am planning to make changes to your fork and then submit a PR to you. I gather those changes will flow through to this PR. Is that correct? Previously, if I wanted to make changes, I just merged the PR and then immediately pushed any minor tweaks as a separate commit.

Anyway, I finished work today for a break over xmas and am going to do the coding for this on some plane journeys. Regardless of whether I push to your fork or merge and then immediately update with a second commit, the end results will be similar. But I see it's best if I push to yours first.

Thanks again for all your efforts and patients. I actually have a group of students who are going to use this functionality as soon as I merge, to simulate 3D printed medical orthotics with FEA. So this might lead to some real-life improvement to patients in the foreseeable future!

ES-Alexander commented 11 months ago

I am planning to make changes to your fork and then submit a PR to you. I gather those changes will flow through to this PR. Is that correct?

Yep - any changes that end up in my mesh-export branch (through me committing them, or through a PR that gets merged into my branch) before this PR gets merged will automatically become part of the PR.

Previously, if I wanted to make changes, I just merged the PR and then immediately pushed any minor tweaks as a separate commit.

That can end up with the same code, but may result in a less clean commit history, and may be a bit harder to track for someone who's looking at this PR later. If neither of those are a concern then merging extra commits later can be fine, so it mostly depends on the nature of the changes you're wanting to make, and whether they conceptually belong as part of this PR :-)

Thanks again for all your efforts and patients.

Hah! I hope this was an intentional medical pun 😆

I actually have a group of students who are going to use this functionality as soon as I merge, to simulate 3D printed medical orthotics with FEA. So this might lead to some real-life improvement to patients in the foreseeable future!

That's awesome - thanks for letting me know!

I'm not sure how I'll be for time over the Christmas/New Year period, but hopefully I'll be able to make some additional code contributions on some of the issues I've raised :-)

fullcontrol-xyz commented 11 months ago

Perfect, I'll do a PR to your branch. I've done the code restructure now, with a new 'transform' option. And will create a quick tutorial notebook or addition to existing tutorials. I've put most of the code in lab.fullcontrol so we can hack around without worrying about end users too much

fullcontrol-xyz commented 11 months ago

Just submitted the pr to your branch, but some comments about some errors to fix. STL output works fine, but the original plot function didn't work for all the tutorial notebooks. Hopefully a pretty easy fix though. Full details given in the pr to your branch

ES-Alexander commented 9 months ago

Your changes have been merged in to my branch (with some minor adjustments), and the colours bug has been fixed within the "refactor" commit, so you're welcome to re-review / merge this PR now when you've got the time to :-)

NOTE: I ended up leaving the colours stuff where you had it in your code, because my mental model of the codebase is still fuzzy enough that it would take a while to think of how that should be meaningfully structured.

  1. If pulling out the colour handling into its own section is something you consider worthwhile for in future but not a blocker for the PR then we can merge this as is and just raise an Issue as a reminder to do it later (and possibly add one or more TODO comments in relevant places in the code - let me know if you want me to do this).
  2. If it's important enough that this shouldn't be merged in its current state then I can try to take another look at it, I just can't guarantee how long it will take to refactor because my free time is quite variable at the moment.
fullcontrol-xyz commented 9 months ago

Excellent thank you, I'm going to try to look at it this week!

fullcontrol-xyz commented 9 months ago

Just merged this, and followed it with an addition commit to update a few of the lab tutorial notebooks as well as temporarily fix an issue in colab (by adding --no-deps to the !pip install command) related to dependencies unnecessarily being attempted to be installed but throwing an error. There are some interesting things in the task list (first post in this pr). I wondered whether to re-open this, but perhaps it's clearer to simply copy and edited version of that first post checklist to a new pr (assuming we can make progress on them). And thanks again for such excellent contributions!!!