gazebosim / gz-math

General purpose math library for robot applications.
https://gazebosim.org/libs/math
Apache License 2.0
54 stars 67 forks source link

Consider removing HasLoop() / EnsureNoLoop() from Spline #74

Closed osrf-migration closed 7 years ago

osrf-migration commented 7 years ago

Original report (archived issue) by Andrés Fortier (Bitbucket: andres_fortier).


After approaching the problem that gave birth to the implementation of HasLoop() and EnsureNoLoop() from a different angle (by using other polynomials to generate the curves) we realized that, instead of checking for loops in Hermite splines, it may be a better option to use a different type of splines from the start (one that can't have loops by definition). In particular, using pchip splines has proven to be a successful approach in avoiding loops altogether, while also ensuring monotonicity and convexity in each curve piece. If we also consider that:

we propose to remove these methods form Spline and instead think of extending ign-math-3 with bezier and pchip curves opportunistically as we need them.

@caguero what do you think?

osrf-migration commented 7 years ago

Original comment by Andrés Fortier (Bitbucket: andres_fortier).


osrf-migration commented 7 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).


Just a heads up that these public functions would need to be deprecated according to the tick-tock release cycle. So they could be deprecated on ign-math 4 (but still function), and only be completely removed on ign-math 5.

osrf-migration commented 7 years ago

Original comment by Andrés Fortier (Bitbucket: andres_fortier).


Great to know that, thanks @chapulina !

osrf-migration commented 7 years ago

Original comment by Carlos Agüero (Bitbucket: caguero, GitHub: caguero).


Have we released a version with these improvements? If not, we probably don't need the deprecations.

osrf-migration commented 7 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).


Humm true, it looks like pull request #169 hasn't been released yet

osrf-migration commented 7 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


Yeah, these changes haven't been released yet.

osrf-migration commented 7 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


Note that there is a memory access error that causes a test failure on osx and valgrind memcheck complaints on Linux (#84). That might be an extra reason to redact the changes from pull request #169.

osrf-migration commented 7 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


I've proposed reverting most of pull request #169 in pull request #197. Feedback is welcome.

osrf-migration commented 7 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


These features were removed in pull request #197.