openscad / openscad

OpenSCAD - The Programmers Solid 3D CAD Modeller
https://www.openscad.org
Other
7.02k stars 1.21k forks source link

STL export should contain single precision floats #2651

Open 3D-ES opened 5 years ago

3D-ES commented 5 years ago

Hello!

I am designing 3D KiCAD PCB parts using OpenSCAD and Wings3D

When I import an OpenSCAD STL file in Wings3D, the program always crashes, after repairing the file using meshconv, the STL file no longer crashes Wings3D.

Meshconv only changed the normal and vertex locations to floating point values:

STL output from OpenSCAD test with cube(10);

solid OpenSCAD_Model
    facet normal -0 0 1
        outer loop
            vertex 0 10 10
            vertex 10 0 10
            vertex 10 10 10
        endloop
    endfacet
    ...

STL output after repairing cube(10) with meshconv:

solid cube
facet normal -0.000000 0.000000 1.000000
    outer loop
        vertex 0.000000 10.000000 10.000000
        vertex 10.000000 0.000000 10.000000
        vertex 10.000000 10.000000 10.000000
    endloop
endfacet
...

The STL file format specifies:

The numerical data in the facet normal and vertex lines are single precision floats, for example, 1.23456E+789.

Currently the STL file contains only integers when testing with a cube(10); After meshconv the value notation is in std::fixed style, so also incorrect?...

I made the following changes in /src/export_stl.cc:

std::string toString(const Vector3d &v)
-   return OSS(v[0] << " " << v[1] << " " << v[2]);
+   return OSS(std::scientific << v[0] << " " << v[1] << " " << v[2]);

void append_stl(const PolySet &ps, std::ostream &output)
if (is_finite(normal) && !is_nan(normal)) {
-       output << normal[0] << " " << normal[1] << " " << normal[2] << "\n";
+       output << std::scientific << normal[0] << " " << normal[1] << " " << normal[2] << "\n";
}
else {
-       output << "0 0 0\n";
+       output << "0.000000e+00 0.000000e+00 0.000000e+00\n";
}

STL output of cube(10) after making the above changes:

solid OpenSCAD_Model
    facet normal -0.000000e+00 0.000000e+00 1.000000e+00
        outer loop
            vertex 0.000000e+00 1.000000e+01 1.000000e+01
            vertex 1.000000e+01 0.000000e+00 1.000000e+01
            vertex 1.000000e+01 1.000000e+01 1.000000e+01
        endloop
    endfacet
    ....

After these changes Wings3D accepts (my very simple) OpenSCAD STL files.

Should the code be changed (this way or another) or is there a reason things are this way? One thing I predict: ASCII STL files will probably grow quite a bit after implementing this...

--- Want to back this issue? **[Post a bounty on it!](https://app.bountysource.com/issues/67971516-stl-export-should-contain-single-precision-floats?utm_campaign=plugin&utm_content=tracker%2F52063&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://app.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F52063&utm_medium=issues&utm_source=github).
MichaelPFrey commented 5 years ago

The STL file format specifies:

Those anyone have the full specification? The article citites as source:

Technical source: StereoLithography Interface Specification, 3D Systems, Inc., October 1989

Outputting the numbers sometimes as integer and sometimes as singles is inconsistent, so the idea of changing it sounds reasonable. Increasing compatibility is a goodthing - but double checking with the standard is also important to avoid changing things back and forward.

MichaelAtOz commented 5 years ago

Note it also says:

The object represented must be located in the all-positive octant. In other words, all vertex coordinates must be positive-definite (nonnegative and nonzero) numbers. 

Nonzero.

Anyway...

Thing is that 'single precision floats' does not a good specification make. As Yoda would say. It specifies 32bit format, not ASCII representation.

I'd suggest given the date of the STL 'standard' (Copyright © 1993, 1999, Ennex Corporation) IEEE754-1985 would have been in effect. Unfortunately, it doesn't specify decimal format. That was in the IEEE 754-2008 revision, it says (5.6.) basically the significand should be an integer So that doesn't help.

Apparently it can be nomalised or not. So 10 is just as good as 1.0x10^1

I have not heard of other S/W crashing with 'proper' OpenSCAD STLs. Did you ask them why they crash? Programs should not crash with 'bad data' so I'd suggest they have a bug.

Significantly increasing ASCII STL file sizes is not a good thing.

MichaelAtOz commented 5 years ago

Those anyone have the full specification?

There is the patent, with a diagram 'FIGS. 39a-39b is the format of the Object. STL file' using normalised format.

wo1989010256 - cad-cam stereolithographic data conversion

MichaelAtOz commented 5 years ago

Apparently they used A Programming Language, 'FIGS. 35a-35c are APL programs for such a model', we don't know what flavour, but IBMs shows: apl numbers

It doesn't explicitly say it's normalised, but we should probably go with the flow/vibe. Perhaps after binary STL export is done? (#1555)

MichaelAtOz commented 5 years ago

When I import an OpenSCAD STL file in Wings3D, the program always crashes,

Umm, Wings only supports binary STLs, OpenSCAD exports ASCII STLs, so @3D-ES you must be converting them, the 'binary' vertex is entirely different to the decimal ASCII, so what conversion program are you using? See also the discussion in #1555 incl.

ASCII decimal floating point and binary floating point are not the same set of numbers. You cannot convert from one back to the other without high risk of losing information

3D-ES commented 5 years ago

@MichaelAtOz

I have not heard of other S/W crashing with 'proper' OpenSCAD STLs. Did you ask them why they crash? Programs should not crash with 'bad data' so I'd suggest they have a bug.

I have tried debugging Wings3D but I don't speak Erlang...

I have analysed the crash, it runs until it starts importing a "list of floating point values".

[{wings,command_1,
     [{'EXIT',
          {badarg,
            [{erlang,list_to_float,["-12"],[]},
               {wpc_stl,'-read_stl_ascii/1-fun-0-',1,
                   [{file,"wpc_stl.erl"},{line,148}]},
                   ...

wings_crash.dump.txt

I agree that Wings3D could probably handle that case better, but the 'bug' is in OpenSCAD.

Umm, Wings only supports binary STLs, OpenSCAD exports ASCII STLs, so @3D-ES you must be converting them, the 'binary' vertex is entirely different to the decimal ASCII, so what conversion program are you using? See also the discussion in #1555 incl.

I am using Wings3D version 2.1.5, and it DOES support ASCII STL. You can try it for yourself with the attached ASCII PowerLED.STL file.

PowerLED.stl.txt

MichaelAtOz commented 5 years ago

I followed the links to GitHub from the wings3D 'official' web site, all I can find is https://github.com/dgud/wings/blob/0b1d1857b40695e45c1e03268edbfafe0d9d86a8/plugins_src/import_export/wpc_stl.erl Which does not do ASCII. I wouldn't be surprised that it is just lucky matching to the ASCII numbers, it is not overly complicated. However your trace does show -read_stl_ascii I don't know where that comes from and googlebing is not helpful, nor a search of 'stl' or 'ascii' in the repository. Is that perhaps part of your system/environment?

The problem appears to be that erlang floats must have a decimal, and hence can't cope with eg '10'. Your trace line number 148 matches to 'X1:32/float-little'

3D-ES commented 5 years ago

My debug / testing environment is a clean Linux Mint 19 in VirtualBox. I have installed Wings3D from the repository and it works out-of-the-box.

To "fix" this "bug", adding ".0" to whole numbers would be enough...

3D-ES commented 5 years ago

I have been digging, the read_stl_ascii function seems to originate from this Debian bugreport: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=698349

Source implementation can be found in this diff: https://salsa.debian.org/erlang-team/packages/wings3d/blob/master/debian/patches/stl.diff

So my Debian-based OS supports ASCII STL, and other platforms probably not?

rewolff commented 5 years ago

@3D-ES The most complicated "number" format example is 1.0E3 for the number "thousand". A program that accepts numbers should accept all possible number formats. So 1E3, 1.0E3, 1000 and 1000.0 . The form "1000", without decimal point is the most compact, why would it be illegal? It is still a valid (and perfectly representable) 32 bit floating point number (whereas 1.1 for example cannot be represented perfectly as a single precision float). It is the parsing program that should simply use a "read-a-float" library function that has support for all possible representations of the number "thousand".

Now whenever parsing input files you somehow think you need to write the number parsing again yourself... think again. Really you should use a proper library function that has been tested across decades and has encoutered all sorts of "unexpected" inputs and has been made robust against that sort of things.

If the author of a program makes the mistake of trying to implement the "read a number" routine again by himself and makes a mistake of not expecting some valid input, then that's a bug in that program.

If OpenScad triggers a bug in another program, then it is nice that OpenScad helps making that other program better. But you should not blame OpenScad for the bugs in that other program.

You can use something as a filter as a workaround for the problems right now, but modifying OpenScad to produce files that happen to work on some random other program is not the way to proceed forward.

That's my opinion.

nophead commented 5 years ago

Also changing OpenSCAD now will not fix all the STL files already on Thingiverse, etc, but fixing Wings3D will make them all readable.

3D-ES commented 5 years ago

@rewolff

Now whenever parsing input files you somehow think you need to write the number parsing again yourself... think again. Really you should use a proper library function that has been tested across decades and has encoutered all sorts of "unexpected" inputs and has been made robust against that sort of things. If the author of a program makes the mistake of trying to implement the "read a number" routine again by himself and makes a mistake of not expecting some valid input, then that's a bug in that program.

I totally agree, but in this case we are talking about the built-in function "list_to_float" of the well respected language Erlang that exists since 1986... That is about 3 decades old... Following your logic means Erlang is right..? ;) Just kidding...

If OpenScad triggers a bug in another program, then it is nice that OpenScad helps making that other program better. But you should not blame OpenScad for the bugs in that other program.

I agree it was a bit harsh from me to call this a bug in OpenSCAD, my apologies.

The only reason I am in this thread is to make things better, currently you need a closed-source program (meshconv) to enable two open-source programs to work together, which is annoying.

@nophead

Also changing OpenSCAD now will not fix all the STL files already on Thingiverse, etc, but fixing Wings3D will make them all readable.

I have asked the Wings3D people to also look into this, trying to fix this "problem" at both sides.

Adding an OpenSCAD export to WRL function starts to look like the easy way out...

Thank you all for your input!

nophead commented 5 years ago

I don't think there is a problem in OpenSCAD. Its STL files are acceptable to all other programs I am aware of.

I don't think I know of any programs that need a decimal point for numbers that are integers to make them acceptable as floats. Erlang must be an odd language if it requires that.

3D-ES commented 5 years ago

Ok, fair.

I stumbled into this problem thinking that the STL format specification I found was the standard. I thought that Wings3D was the only program that followed it to the letter, and crashed because of it.

I do agree it is a bit odd to crash over a float value without a dot in it, I don't know why that happens.

It is probably best if I lay this to rest for a while, hopefully Wings3D can be made more flexible.

MichaelAtOz commented 5 years ago
     case file:read_file(Name) of
+    {ok,<<"solid", Bin/binary>>} ->
+   print_boxed("FileName: " ++ Name),
+   {Vs,Fs} = read_stl_ascii(Bin),
+   Res = import(Vs,Fs),
+   Res;
     {ok,Bin} ->
    print_boxed("FileName: " ++ Name),
+   {Vs,Fs} = read_stl(Bin),
+   Res = import(Vs,Fs),
    Res;
     {error,Reason} ->
    {error,file:format_error(Reason)}
     end.

My googlebingbabblefish translates that to

IF LEFT(FILE,5) == "solid" THEN read_stl_ascii(RIGHT(FILE,-5)) ELSE read_stl(FILE)

Seems the author had one Solidworks(?) ASCII STL file to work with...

@3D-ES if you have the time, edit your STL, make the first 5 chars 'solid' give it a go.

MichaelAtOz commented 5 years ago

No strike that. ASCII STL all start with 'solid' - engage brain first next time. ENGAGE.

MichaelAtOz commented 5 years ago

The regular expression is just parsing on whitespace/non-whitespace, so it does look like it is purely list_to_float() not able to handle an integer. So than annon. fun() needs to be smarter, but I think I've hit my erlang googlebingbabblefish® limits...

t-paul commented 4 years ago

The patent @MichaelAtOz linked in https://github.com/openscad/openscad/issues/2651#issuecomment-450046485 (which is by 3D Systems Inc.) has a page right at the end (6th last?) which shows an example file with just vertex 0 1 0. However it says at the bottom "Note: ASCII Floating Point Notation is typically expected for normal and vertex coordinate values, e.g. "6.58537e-10". I'll tag this as bug for now.

bubnikv commented 3 years ago

IMHO openscad has quite an issue due to insufficient number of decimal digits stored into an ASCII STL file, which is most likely a source of a plethora of issues at our poor PrusaSlicer:

https://github.com/prusa3d/PrusaSlicer/issues?q=is%3Aopen+is%3Aissue+label%3A%22model+import%22

It looks as if openscad exports floats into std::stream using the default precision, which is just 6 digits only, but we need at least 9 for a lossless round trip conversion to / from string. This is achieved by

stream << std::setprecision(std::numeric_limits<float>::max_digits10);

though only if neither "float" nor "scientific" format is chosen, but the stream is left in the "default" state. This is indeed the way we store float coordinates in PrusaSlicer. The same accuracy is achieved with

sprintf(buf, "%.9g", f);

at least on Visual Studio I get binary equivalent strings after the conversion from both methods.

The "%.9g" specifier means "pick the shortest of scientific or decimal" using maximum 9 decimal digits, but drop the trailing zeros. It does exactly what the doctor prescribed.

t-paul commented 3 years ago

@bubnikv Sorry, STL is defined to be single precision (note the title of the issue?). Yes this causes issues for OpenSCAD too, but exporting something else basically means it's not STL anymore.

The only option for that is probably advocating 3MF more which does support higher precision (and lib3mf supports that in V2 via https://github.com/3MFConsortium/lib3mf/pull/169).

bubnikv commented 3 years ago

@t-paul

Sorry, STL is defined to be single precision (note the title of the issue?). Yes this causes issues for OpenSCAD too, but exporting something else basically means it's not STL anymore.

Single precision float requires 9 decimal digits precision for lossless round trip conversion to / from string.

t-paul commented 3 years ago

Well, ok, Wikipedia gives that interesting statement (it should match?)

If a decimal string with at most 6 significant digits is converted to IEEE 754 single-precision representation, and then converted back to a decimal string with the same number of digits, the final result should match the original string. If an IEEE 754 single-precision number is converted to a decimal string with at least 9 significant digits, and then converted back to single-precision representation, the final result must match the original number.[

bubnikv commented 3 years ago

I suppose that statement is correct. First statement says you need 6 digits when converting a 6 digit text -> float -> 6 digit text. The second statement says you need 9 digits for converting float -> 9 digits text -> float for a lossless round trip conversion.

nophead commented 3 years ago

It basically says you can only represent all combinations of 6 digits in binary but that doesn't used all binary bit combinations. To represent all combinations of binary in ASCII you need 9 digits but not all 9 digit combinations.

Seems like this might fix a lot of problems with STLs becoming degenerate on export.

t-paul commented 3 years ago

Maybe, but fiddeling with file formats can also cause lots of trouble, especially with STL which does not even have a clear specification. One option could be changing it for the upcoming dev cycle and see if it would cause import issues somewhere. But I'm not liking that option very much.

nophead commented 3 years ago

Seems like it is correcting a bug though. The 9 digit values would only include those that fit in a single precision float, so readers should accept them.

nophead commented 3 years ago

Single precision float is defined by the binary bits it has, not the number of decimal digits.

t-paul commented 3 years ago

True, but check the original post of this issue. Implementation don't always agree on details.

nophead commented 3 years ago

Have you even known a program to complain if a float has too many digits after the point? I don't recall any complaints like that. I think at worst they would get ignored.

I presume there are lots of broken readers out there but I don't think that means we should have a broken writer as well.

t-paul commented 3 years ago

I would not have considered an importer failing to read 10 as float either, so that's not really the point. I gave my view on how this could move forward already. Still needs someone to find the time to implement the change.

bubnikv commented 3 years ago

Seems like this might fix a lot of problems with STLs becoming degenerate on export.

Exactly.

I suppose we are lucky and the floats are rounded to 6 digits consistently, so no cracks are created in a manifold, but degenerate triangles will be created (not a big deal, the consumer could remove them quite easily), and self-intersections will be created. The self intersections are a huge problem for consumers expecting manifold models, like a 3D printing slicer or a CSG modeler. A one-liner change of adding

stream << std::setprecision(std::numeric_limits<float>::max_digits10);

after creating the std::stream should fix it.

rewolff commented 3 years ago

STLs are single precisison by definition. That's the binary format.

But what I didn't know until a few moments ago is that Openscad writes ascii STLs ! Edit: Strike that: "what I had forgotten until a few moments ago.... " :-)

In that case, what is preventing openscad from working with (much) higher precision internally and then outputting a few (according to the STL definition) irrelevant digits?

A few extra digits are very very unlikely to cause problems with consumers of the ascii files. Who is writing their own float-parsing-code and decides to stop parsing after 6 digits??? Standard (stdlib) float parsing code will handle it correctly.

If we don't trust that others handle this correclty, the "how many digits" can become a configuration option, or another "improved, possibly not standards compliant STL version" output option could be added. Both is more work than one line, probably.

tombsar commented 3 years ago

Does anyone actually want ASCII STL output? Wouldn't this issue be solved by replacing it with binary STL?

t-paul commented 3 years ago

OpenSCAD now supports both and exports binary STL by default when running in GUI mode (with a config switch to go back to ASCII for people who need that).

rewolff commented 3 years ago

Ohhh. I looked at an STL file that I designed last weekend to check the format. It's quite possible that I'm running an older version.

bubnikv commented 3 years ago

In that case, what is preventing openscad from working with (much) higher precision internally and then outputting a few (according to the STL definition) irrelevant digits?

That is already being done. OpenSCAD is based on CGAL, which achieves its numerical stability by using exact arithmetic with filtered predicates, thus the internal representation of the vertices uses a variable number of bits, which then need to be truncated to floats when exporting STLs. This truncation may create degeneracies or self intersection.

Now when exporting ASCII STLs to 6 digits instead of 9, we are increasing the risk of degeneracies by 2 to 3 orders of magnitude.

tombsar commented 3 years ago

Ohhh. I looked at an STL file that I designed last weekend to check the format. It's quite possible that I'm running an older version.

I made the same mistake; it was added in https://github.com/openscad/openscad/pull/3381, after the last stable release, so you need to be using dev snapshots or release candidates to get it.

I see no reason not to increase precision to 9 decimal digits. Anyone who has opted-in to ASCII STL doesn't care about filesize anyway.

jordanbrown0 commented 3 years ago

Concur that it would be very surprising to find that an ASCII consumer was upset by getting too many digits. In fact, it would not be at all surprising to find consumers using doubles internally, because C prefers doubles. For instance, in a quick trek through Cura, it appears that Cura uses NumPy-STL to read STL files, and that uses the native Python float() function, and Python documentation says that floating point is usually implemented using C double.

t-paul commented 3 years ago

I do appreciate how everyone seems to think it's impossible to break parsing floats while the main topic of the ticket is failing to parse 10 as float.

Long story short and as said above already, we can try if it causes issues in the dev snapshots for a while.

rewolff commented 3 years ago

@t-paul After my last comment I went back to read some of the "context" and came to realize that....

Still like I said before, an implementation for "reading numbers into a floating point variable" should accept as much variable input as possible. Because 2 != 10, the amount of precision you get in a floating point number varies depending on where in the decade you end up. 1.234 is less accurate than 9.876. Because of that, maybe 6 digits is enough to represent the 24-bit mantissa in a 32-bit float, but someone might have seen a fringe case, or wants to be "sure" and print 7 digits. Now you fail to parse? Bullshit! Bad implementation! Similarly for the "10" case. Suppose I write an application and ask for a percentage. 10, 20, 45 great! So then one user comes and asks: I need to enter a decimal after the percentage, the percentage is internally a float already, so I want to be able to enter 19.5... So I change the input routine to parse the number directly into a float variable and now suddenly the "10" and "20" users end up with a parse error??? In mathematics the integers are a subset of "reals". In computer science, floating point variables can hold integers (and within a certain range, represent them exactly).

If your language only ever "interacts" with itself for printing and reading back "float" variables, then you might overlook having to parse 123456 as a valid float But once you run into that one, the language/library needs to be fixed to correctly handle that kind of input.

So.... for the problem at hand: STL input for erlang-language-tools needs to be filtered through a tool that prints the floats in a way that Erlang groks. And the users of those tools should complain to their language maintainers that this is annoying and should be unnecessary. That's the way forward. Keeping other tools back only to remain compatible with buggy implementations is a great way to create a big mess.

3D-ES commented 3 years ago

So.... for the problem at hand: STL input for erlang-language-tools needs to be filtered through a tool that prints the floats in a way that Erlang groks. And the users of those tools should complain to their language maintainers that this is annoying and should be unnecessary. That's the way forward.

I agree, and I did, this bug was also reported to the Wings3D developers and has been fixed there, it is no longer an issue.

Sorry for any inconvenience this bug report has caused, I should have reported it there first.