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

8241582: Infinite animation does not start from the end when started with a negative rate #169

Conversation

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Apr 10, 2020

Cause

Animation#jumpTo(Duration) does not handle Duration.INDEFINITE properly. This causes InfiniteClipEnvelope#jumpTo(long) to receive an erroneous value and start the animation not from the end, depending on the modulo calculation.

Fix

For infinite cycles, use the cycleDuration as the destination since that is the effective end point.

Tests

  • Added an testJumpTo_IndefiniteCycles test that fails without the patch and passes after it.
  • Added a testJumpTo_IndefiniteCycleDuration that passes both before and after, just to make sure that this type of infinite duration is correct.
  • Removed a test in SequentialTransition that fails with this patch, but it passed before it only because the modulo value happened to land in the right place. Changing the duration of one of the child animation can cause it to fail, so the test is faulty anyway at this stage.

Future work

Playing backwards will still not work correctly, but at least now it start from the correct value. This is the first of a series of fixes under the umbrella issue JDK-8210238.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8241582: Infinite animation does not start from the end when started with a negative rate

Reviewers

  • Ambarish Rapte (arapte - Reviewer)
  • Kevin Rushforth (kcr - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/169/head:pull/169
$ git checkout pull/169

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 10, 2020

👋 Welcome back nlisker! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Apr 10, 2020
@nlisker nlisker changed the title 8241582: infinite animation does not start from the end when started with a negative rate 8241582: Infinite animation does not start from the end when started with a negative rate Apr 10, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 10, 2020

Webrevs

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 14, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 14, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@arapte
arapte approved these changes Apr 16, 2020
Copy link
Member

@arapte arapte left a comment

looks good to me.

@kevinrushforth kevinrushforth self-requested a review Apr 21, 2020
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good. I left a question about one of the tests (for my own curiosity), but I'm approving it anyway.

animation.shim_setCycleDuration(Duration.INDEFINITE);

animation.jumpTo("end");
assertEquals(Duration.millis(Long.MAX_VALUE / 6), animation.getCurrentTime());

This comment has been minimized.

This comment has been minimized.

@nlisker

nlisker Apr 22, 2020
Author Collaborator

This is a good question and I think I should have explained it in a comment because it also points to a mini-flaw. The short answer is that the 6 comes from the TicksCalculation class, which defines TICKS_PER_SECOND = 6000 and TICKS_PER_MILI = TICKS_PER_SECOND / 1000.0 which is 6.

The test works as follows:

This is the "ticks from duration" calculation for jumping to the end (Duration.INDEFINITE), demonstrated by mathematical substitutions:

double millis = Duration.INDEFINITE.toMillis() = Double.POSITIVE_INFINITY
long ticks = TickCalculation.fromMillis(millis) = Math.round(TICKS_PER_MILI * millis)
        = Math.round(6 * Double.POSITIVE_INFINITY) = Math.round(Double.POSITIVE_INFINITY) 
        = Long.MAX_VALUE (as per the spec. of Math.round)

Notice that we lose precision when multiplying POSITIVE_INFINITY.

Then getting the current time calculates "duration from ticks":

animation.getCurrentTime() = TickCalculation.toDuration(currentTicks)
        = ticks / TICKS_PER_MILI = Long.MAX_VALUE / 6

So the division carries out normally (there's no Long.POSITIVE_INFINITY), but the multiplication does not.

Maybe we shouldn't convert between the double-based Duration and the long ticks so much, but I think that this is somewhat negligible.

My next patch is refactoring in preparation of the next, heavier, changes, so I will add this explanation on the test.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 22, 2020

@nlisker This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8241582: Infinite animation does not start from the end when started with a negative rate

Reviewed-by: arapte, kcr
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 23 commits pushed to the master branch. As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate dedf7cb73a012e74b098cc49330bc00c20b97bac.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Apr 22, 2020
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Apr 22, 2020

/integrate

@openjdk openjdk bot closed this Apr 22, 2020
@openjdk openjdk bot added the integrated label Apr 22, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Apr 22, 2020

@nlisker The following commits have been pushed to master since your change was applied:

  • dedf7cb: 8242490: Upgrade to gcc 9.2 on Linux
  • 5e9fb82: 8242577: Cell selection fails on iOS most of the times
  • 69e4266: 8242489: ChoiceBox: initially toggle not sync'ed to selection
  • 1d88180: 8243112: Skip failing test SVGTest.testSVGRenderingWithPattern
  • ec8608f: 8223298: SVG patterns are drawn wrong
  • e82046e: 8242530: [macos] Some audio files miss spectrum data when another audio file plays first
  • 7044cef: 8241476: Linux build warnings issued on gcc 9
  • 9d50c4c: Merge
  • 4d69a0d: 8241629: [macos10.15] Long startup delay playing media over https on Catalina
  • b1fdc45: 8242209: Increase web native thread stack size for x86 mode
  • e1cb191: 8240694: [macos 10.15] JavaFX Media hangs on some video files on Catalina
  • ef37669: Merge
  • f2bca9f: Merge
  • 6900d29: Merge
  • e91bec4: Merge
  • 66a8f49: Merge
  • fde42da: Merge
  • e21fd1f: Merge
  • 443c845: Merge
  • 31e63de: Merge
  • 14c6938: 8236798: Enhance FX scripting support
  • bfb2d0e: Merge
  • 39f6127: Merge

Your commit was automatically rebased without conflicts.

Pushed as commit 48476eb.

@openjdk openjdk bot removed ready rfr labels Apr 22, 2020
@nlisker nlisker deleted the nlisker:8241582_Infinite_animation_does_not_start_from_the_end_when_started_with_a_negative_rate branch Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants