msteinbeck / tinyspline

ANSI C library for NURBS, B-Splines, and Bézier curves with interfaces for C++, C#, D, Go, Java, Javascript, Lua, Octave, PHP, Python, R, and Ruby.
MIT License
1.2k stars 208 forks source link

Precision #65

Closed resema closed 8 years ago

resema commented 8 years ago

Hi Marcel

Another question: I have source data which needs double precision. I have seen that there is a definition for max. relative and absolute errors. If I would change everything to double and change the precision accordingly, do I get some problems with that?

Kind regards, Renato

msteinbeck commented 8 years ago

If you use the C/C++ interface, replacing floats with doubles should be feasible. However, the bindings assume floats and will not be generated properly if you use doubles instead. I think I will add support for both datatypes because it has been requested a couple of times.

resema commented 8 years ago

Okay, I though of changing the bindings and fequals and the rest as well... I have some position data in the format WGS84 for world coordinates, which are pretty small... =)

msteinbeck commented 8 years ago

Which interface are you using?

resema commented 8 years ago

I'm using the c++ wrapper

msteinbeck commented 8 years ago

Then it should be straight forward. Replace all float with double and std::vector<float> with std::vector<double>.

resema commented 8 years ago

Since my delta's between the points are around 4th to the 8th position after the point, e.g. 47.84560 and 47.84574. I assume, I have to change the FLT_MAX_ABS_ERROR and FLT_MAX_REL_ERROR as well right?

msteinbeck commented 8 years ago

I have to change the FLT_MAX_ABS_ERROR and FLT_MAX_REL_ERROR as well right?

If you use ts_fequals to compare your control points, yes. Otherwise, this function is used for comparing the knot values of a spline only and, thus, must not be adjusted when switching from float to double.

resema commented 8 years ago

Okay, do I understand it right, that I have to change the comparision function ts_fequals as soon as my control points have a smaller difference than the absolute error?

msteinbeck commented 8 years ago

Currently, ts_fequals is supposed to compare knot values only. If Tinyspline requires a comparing functions for control points a separate function will be added.

resema commented 8 years ago

I understand! Thanks a lot! I will make some further tests! I have to say, this is a nice tiny lib =)

msteinbeck commented 8 years ago

I have to say, this is a nice tiny lib =)

Thanks :).

msteinbeck commented 8 years ago

I will make some further tests!

Let me know if you successfully replaced floats with doubles.

resema commented 8 years ago

Of course!

resema commented 8 years ago

Hey Marcel

I hadn't much time in the last weeks, but now I have ported the lib to double precisions. And it looks like it works as expected...

Kind regards, Renato

msteinbeck commented 8 years ago

Nice to know, thanks. I will introduce a typedef in future to allow users configure whether float or double should be used.

giumas commented 8 years ago

I will introduce a typedef in future to allow users configure whether float or double should be used.

@retuxx : I do encourage you on doing it! The @rsemadeni solution of doing the port to double precision (that I have also adopted) is not so maintainable.

msteinbeck commented 8 years ago

I just pushed some changes introducing a new typedef, namely tsRational. For now, you have to replace typedef float tsRational; with typedef double tsRational; to have double precision support for the C and C++ interface. However, the examples and bindings will fail to compile since they need some more complex modifications in order to work with double. Additionally, after finished the CMakeLists changes I'm currently working on, I will add a define allowing you to specify the desired precision from cmake without the necessity of changing the source.

giumas commented 8 years ago

what is the current status of this? did you add the option in the CMakelists?

msteinbeck commented 8 years ago

I'm about to push a set of commits adding an option to build TinySpline either with float or with double precision.

giumas commented 8 years ago

Eager to test the oncoming changes

msteinbeck commented 8 years ago

I just pushed a preview of the enhanced CMake files. You may export the environment variable TINYSPLINE_DOUBLE_PRECISION=yes|true|1 (the usual CMake values to enable a variable) or define it directly with CMake: cmake -DTINYSPLINE_DOUBLE_PRECISION=yes|true|1 to build all interfaces with double precision support. Furthermore, I was working on #47 by explicitly setting the output directory of the libraries and binaries regardless of the compiler suite. All library related files will be stored in the directory lib, all binary related files in bin. Thus, setup.py and pom.xml should properly copy the bindings with all suits.

Eager to test the oncoming changes

I would be appreciated if you could test the files on Windows. However, the next steps will be adding a summary that is printed the end of the CMake run showing the actual used configuration. Since I have some free time the next week, I will increasingly work on TinySpline adding new features such as knot removal, better memory management etc.

giumas commented 8 years ago

Wouldn't better with a CMake option?

giumas commented 8 years ago

About the GLUT example, converting everything to double seems to work with the exception of gluNurbsCurve that needs GLFloats.

Do you have a better solution than adding something like this to the display methods?

GLfloat* knots = new GLfloat[spline.n_knots];
for(int i = 0; i < spline.n_knots; ++i)
{
    knots[i] = GLfloat( spline.knots[i] );
}

GLfloat* ctrlp = new GLfloat[spline.n_ctrlp * spline.dim];
for(int i = 0; i < spline.n_ctrlp * spline.dim; ++i)
{
    ctrlp[i] = GLfloat( spline.ctrlp[i] );
}
[...]
gluNurbsCurve(
    theNurb,
    (GLint)spline.n_knots,
    knots,
    (GLint)spline.dim,
    ctrlp,
    (GLint)spline.order,
    GL_MAP1_VERTEX_4
    );
[...]
delete[] knots;
delete[] ctrlp;
msteinbeck commented 8 years ago

Do you have a better solution than adding something like this to the display methods?

There is not support for double based splines in GLUT. However, subdividing a spline into its bezier segments and drawing them one after another with OpenGL is feasible. I could add one or two examples using this approach.

msteinbeck commented 8 years ago

Wouldn't better with a CMake option?

So what's the difference? If I understand right, you have to enable/disable an option with -DOption=On. This is already supported.

giumas commented 8 years ago

However, subdividing a spline into its bezier segments and drawing them one after another with OpenGL is feasible.

I am not sure that this solution is less cumbersome that my approach of recreating the C array as floats. It could be applied only when TINYSPLINE_DOUBLE_PRECISION is defined.

giumas commented 8 years ago

So what's the difference?

The difference is that you get the option to flag/unflag when you run the CMake gui:

cmake-gui_2016-08-20_20-32-36

This is how all the other CMake-based libraries deal with this kind of stuff: http://docs.opencv.org/2.4/doc/tutorials/introduction/windows_install/windows_install.html#building-the-library

msteinbeck commented 8 years ago

The difference is that you get the option to flag/unflag when you run the CMake gui:

The option has been added in newest commit. Thanks for the hint, never used the GUI and, therefore, didn't know about this feature.

msteinbeck commented 8 years ago

I am not sure that this solution is less cumbersome that my approach of recreating the C array as floats.

Since ctrlp and knots are stored in a single array (https://github.com/retuxx/tinyspline/blob/master/library/tinyspline.h#L97) you only need to create an array of size n_ctrlp + n_knots and memcpy the values from one to another.

It could be applied only when TINYSPLINE_DOUBLE_PRECISION is defined.

I would prefer adding one or two examples using your approach instead of ifdefing the existing one.

msteinbeck commented 8 years ago

I'm closing this issue since the remaining request perfectly fits to: #58