Total time updaters #1520

AntonBallmaier · 2021-05-15T23:23:56Z

Changelog / Overview

Mobject.add_updater has a new parameter pass_relative_times determining if the updater should get passed relative or absolute time values.
Updaters can now have arbitraty named second parameters instead of requiring dt.
call_updater=True no longer causes an error with non-time-based updaters.
Renamed Mobject.update to Mobject._apply_updaters and deprecated update.

Motivation

From time to time you don't need the time difference since last updater call, but the total time... Now you without a problem.
Closes #321
Includes parts of the feature requested in #851

Explanation for Changes

Documentation Reference

Checklist

I have read the Contributing Guidelines
I have written a descriptive PR title (see top of PR template for examples)
I have written a changelog entry for the PR or deem it unnecessary
My new functions/classes either have a docstring or are private
My new functions/classes have tests added and (optional) examples in the docs
My new documentation builds, looks correctly formatted, and adds no additional build warnings

Reviewer Checklist

The PR title is descriptive enough
The PR is labeled correctly
The changelog entry is completed if necessary
Newly added functions/classes either have a docstring or are private
Newly added functions/classes have tests added and (optional) examples in the docs
Newly added documentation builds, looks correctly formatted, and adds no additional build warnings

-> update is deprecated

for more information, see https://pre-commit.ci

…er/manim into total_time_updaters

for more information, see https://pre-commit.ci

jsonvillanueva

pre-commit is complaining about these -- applying these changes

manim/mobject/mobject.py

manim/mobject/opengl_mobject.py

Applied pre-commit suggestions

jsonvillanueva

In general, this PR functions as intended from my tests/usage of the new feature. I'll approve now as it works and this could be added to the 0.7.0 release, but I've offered some suggestions to consider.

jsonvillanueva · 2021-05-31T01:58:47Z

manim/mobject/mobject.py

+        pass_relative_times
+            Whether the time information passed to the updater should be relative
+            (usually the time since last call) or absolute (sum of all relative times
+            since the updater was added). If ``pass_relative_times`` is ``None``
+            relative times get passed, except if the second parameter of the updater is
+            named ``t`` or ``time``.


I'd rather pass_relative_times have a default argument of True than be None and later make it False if necessary. Also, I don't particularly like the feature of changing the updater to total time if the second argument is t /time... seems a bit hacky.

jsonvillanueva · 2021-05-31T01:59:55Z

manim/mobject/mobject.py

-        update_function: Updater,
+        updater: Updater,
+        *,
+        pass_relative_times: Optional[bool] = None,


Suggested change

pass_relative_times: Optional[bool] = None,

pass_relative_times: optional[bool] = True,

jsonvillanueva · 2021-05-31T02:02:17Z

manim/mobject/mobject.py

+            if parameters[1] not in ["t", "time"] and pass_relative_times is None:
+                pass_relative_times = True
+            pass_relative_times = bool(pass_relative_times)


again, I don't particularly like this hard coded check for the second argument, but with a default of True, you can simplify the logic to only check for if it should be set to False

Suggested change

if parameters[1] not in ["t", "time"] and pass_relative_times is None:

pass_relative_times = True

pass_relative_times = bool(pass_relative_times)

if parameters[1] in ["t", "time"]:

pass_relative_times = False

Darylgolden · 2022-01-14T01:54:57Z

Hi, are you still planning on working on this?

MrDiver · 2022-06-18T17:58:44Z

We will come back to this with a fresh view if the issue get's relevant again. This PR is already linked in the original issue

Deprecate different signatures for updaters #321

So i will close this for now. Feel free to open this PR again or a new PR which fixes the issue if you want to work on it again!

AntonBallmaier added 3 commits May 15, 2021 19:17

adjust mobject methods and remove get_time_based_updaters

255ebe2

renamed Mobject.update to _apply_updaters

06715b6

-> update is deprecated

apply changes to opengl mobject

a030b43

AntonBallmaier added pr:bugfix Bug fix for use in PRs solving a specific issue:bug pr:deprecation Deprecation, or removal of deprecated code new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) labels May 15, 2021

pre-commit-ci bot and others added 3 commits May 15, 2021 23:24

[pre-commit.ci] auto fixes from pre-commit.com hooks

c8d4f2e

for more information, see https://pre-commit.ci

fix filter synax error

7f61bfc

Merge branch 'total_time_updaters' of https://github.com/AntonBallmai…

3aee0db

…er/manim into total_time_updaters

AntonBallmaier mentioned this pull request May 17, 2021

Reworking the update module #1538

Open

Merge branch 'master' into total_time_updaters

b7c4586

AntonBallmaier requested a review from jsonvillanueva May 27, 2021 11:15

AntonBallmaier and others added 3 commits May 27, 2021 13:16

Merge branch 'master' into total_time_updaters

e8aa5a2

Merge branch 'master' into total_time_updaters

b2bb949

[pre-commit.ci] auto fixes from pre-commit.com hooks

a07b5ea

for more information, see https://pre-commit.ci

jsonvillanueva reviewed May 30, 2021

View reviewed changes

manim/mobject/mobject.py Outdated Show resolved Hide resolved

manim/mobject/mobject.py Outdated Show resolved Hide resolved

manim/mobject/opengl_mobject.py Outdated Show resolved Hide resolved

Apply suggestions from code review

886c7ae

Applied pre-commit suggestions

jsonvillanueva approved these changes May 31, 2021

View reviewed changes

Darylgolden mentioned this pull request Jan 9, 2022

Split add_updater into add_updater and add_dt_updater #2452

Open

MrDiver closed this Jun 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Total time updaters #1520

Total time updaters #1520

Uh oh!

AntonBallmaier commented May 15, 2021 •

edited by jsonvillanueva

Loading

Uh oh!

jsonvillanueva left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsonvillanueva left a comment

Uh oh!

jsonvillanueva May 31, 2021 •

edited

Loading

Uh oh!

jsonvillanueva May 31, 2021

Uh oh!

jsonvillanueva May 31, 2021

Uh oh!

Darylgolden commented Jan 14, 2022

Uh oh!

MrDiver commented Jun 18, 2022

Uh oh!

Uh oh!

	pass_relative_times: Optional[bool] = None,
	pass_relative_times: optional[bool] = True,

Total time updaters #1520

Total time updaters #1520

Uh oh!

Conversation

AntonBallmaier commented May 15, 2021 • edited by jsonvillanueva Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog / Overview

Motivation

Explanation for Changes

Checklist

Reviewer Checklist

Uh oh!

jsonvillanueva left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsonvillanueva left a comment

Choose a reason for hiding this comment

Uh oh!

jsonvillanueva May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsonvillanueva May 31, 2021

Choose a reason for hiding this comment

Uh oh!

jsonvillanueva May 31, 2021

Choose a reason for hiding this comment

Uh oh!

Darylgolden commented Jan 14, 2022

Uh oh!

MrDiver commented Jun 18, 2022

Uh oh!

Uh oh!

AntonBallmaier commented May 15, 2021 •

edited by jsonvillanueva

Loading

jsonvillanueva left a comment •

edited

Loading

jsonvillanueva May 31, 2021 •

edited

Loading