Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: removed boost dependency #2792

Merged
merged 3 commits into from Aug 31, 2022
Merged

refactor: removed boost dependency #2792

merged 3 commits into from Aug 31, 2022

Conversation

rodolforg
Copy link
Contributor

implement our own Runge-Kutta54 Cash-Karp, that boost::ode
uses in integrate() as claimed in documentation:
https://www.boost.org/doc/libs/1_66_0/libs/numeric/odeint/doc/html/boost_numeric_odeint/getting_started/short_example.html

Algorithm described here
http://www.elegio.it/mc2/rk/doc/p201-cash-karp.pdf

As far as I tested, it gives the same results.

@rodolforg rodolforg marked this pull request as ready for review August 12, 2022 21:06
@lgtm-com
Copy link

lgtm-com bot commented Aug 12, 2022

This pull request introduces 1 alert when merging ce77587 into 0a545cc - view on LGTM.com

new alerts:

  • 1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

@ice0
Copy link
Collaborator

ice0 commented Aug 14, 2022

@morevnaproject

I created a file with some random values using a "dynamic" converter and tested it with the current PR and master branch.
test-dynamic.zip

cmp -l ./test-dynamic.avi ./test-dynamic-boost.avi shows no difference.

Do you plan to test it yourself or can we merge it?

@morevnaproject
Copy link
Member

Thank you! I am going to test it too now. ^__^

@morevnaproject
Copy link
Member

I have finished testing and it gives exactly the same results for me too! Awesome work.

I also checked examples from this page -
https://wiki.synfig.org/Convert#Dynamic

Please rebase and we are ready to merge!

@rodolforg
Copy link
Contributor Author

rebased

autobuild/build.sh Outdated Show resolved Hide resolved
implement our own Runge-Kutta54 Cash-Karp, that boost::ode
uses in integrate() as claimed in documentation:
https://www.boost.org/doc/libs/1_66_0/libs/numeric/odeint/doc/html/boost_numeric_odeint/getting_started/short_example.html

Algorithm described here
http://www.elegio.it/mc2/rk/doc/p201-cash-karp.pdf

As far as I tested, it gives the same results.
it seems the previous implementation sometimes enter in an infinite loop

In my tests, however, some parts of this new code are never reached.
That's the reason the code could be simplified to only this `if` branch
```c++
		if (e_4 > 1) {
			// No order less than 5th order is possibly good, so abandon current step
			Real esttol = e_4;
			step_size *= std::max(0.2, VRKF_sf / esttol);
			continue;
		} else {
			// accept 5th order solution
			...
		}
```

And consequently some variables could vanish.
read previous commit message
@rodolforg
Copy link
Contributor Author

Done :)

@ice0 ice0 changed the title refactor: remove boost dependency refactor: removed boost dependency Aug 31, 2022
@ice0 ice0 merged commit 18b9d2b into synfig:master Aug 31, 2022
@ice0
Copy link
Collaborator

ice0 commented Aug 31, 2022

Merged. Thank you!

@rodolforg rodolforg deleted the bye-boost branch August 31, 2022 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants