search

AllwineDesigns / stl_cmd

stl_cmd - commands for binary STL file manipulation
Other
50 stars 9 forks source link

Multifile support for stl_bbox, added stl_zero #6

Closed acwest closed 4 years ago

acwest commented 4 years ago

stl_zero centres an stl file (optionally with the bottom at z = 0. Also added some support for ascii STL files

jallwine commented 4 years ago

Hey @acwest, thanks for the contribution! I took a quick look through and it looks good for the most part. When compiling, I saw a huge number of warnings and it looks like you need to be more careful with your naming of the facet type vs the facet variable. Specifically, the read_facet function in src/stl_util.h, the facet variable hides the facet type, so on line 189 you aren't properly zeroing the memory allocated for a facet. For the most part, that probably won't cause issues, but it could cause subtle issues down the road. Consider renaming the facet type to facet_t or avoid using the variable name facet. I'll happily merge your changes after you address that concern.

acwest commented 4 years ago

Whoops, I noticed that myself, I thought I had fixed them all, but apparently not... I like the idea of using _t. It was my first day with your code, it was exactly what I needed :-) My C and C++ is a little rusty lately, it's good to get back into it, although most of this codebase seems to be using C features only....

jallwine commented 4 years ago

When I started the repository, I thought going with only C would be a good idea, but operator overloading and other C++ features are nice and when I started porting CSG.js, C++ came in handy. Maybe one day I'll try to clean things up and go all in with C++.

acwest commented 4 years ago

I can work with either, really. Sorry about arbitrary whitespace changes, sometimes the urge is irresistible...