Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@cbracken
Copy link
Member

@cbracken cbracken commented Sep 6, 2020

Description

This updates the web_ui implementation of lerpDouble to match the
behaviour of the C++ engine implementation in dart:ui.

Specifically this covers the following changes:

Tests

This adds tests for the web_ui implementation of lerpDouble.

Related Patches

lerpDouble: stricter handling of NaN and infinity (#20871)

Previously, the behaviour of lerpDouble with respect to NaN and infinity
was relatively complex and difficult to reason about. This patch
simplifies the behaviour with respect to those conditions and adds
documentation and tests.

In general, if a == b or both values are null, infinite, or NaN, a
is returned. Otherwise we require a and b and t to be finite or
null and the result of the linear interpolation is returned.

Improve the precision of lerpDouble (#20879)

Reduces errors caused by the loss of floating point precision when the
two extrema of the lerp differ significantly in magnitude. Previously,
we used the calculation:

a + (b - a) * t

When the difference in magnitude between a and b exceeds the
precision representable by double-precision floating point math, b - a
results in the larger-magnitude value of a or b. The error between
the value produced and the correct value is then scaled by t.

A simple example of the impact can be seen when a is significantly
larger in magnitude than b. In that case, b - a results in a and
when t is 1.0, the resulting value is a - (a) * 1.0 == 0.

The patch transforms the computation to the mathematically-equivalent
expression:

a * (1.0 - t) + b * t

By scaling each value independently, the behaviour is more accurate.
From the point of view of performance, this adds an extra
multiplication, but multiplication is relatively cheap and the behaviour
is significantly better.

This patch also adds a precisionErrorTolerance constant to
test_utils.dart and migrates existing tests to use closeTo() for
testing.

The tests themselves do currently use values that have an exact
floating-point representation, but we should allow for flexibility in
future implementation changes.

@cbracken cbracken added the Work in progress (WIP) Not ready (yet) for review! label Sep 6, 2020
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@cbracken cbracken requested a review from yjbanov September 7, 2020 02:34
@cbracken cbracken removed the Work in progress (WIP) Not ready (yet) for review! label Sep 7, 2020
@cbracken
Copy link
Member Author

cbracken commented Sep 7, 2020

@yjbanov happy to add tests like I did with the dart:ui side, but not sure what the standard approach is here -- do you just copy/paste then patch the dart:ui tests? If so, is there any mechanism to ensure they don't get out of sync when behaviour changes?

@cbracken cbracken requested a review from ferhatb September 8, 2020 16:52
Copy link
Contributor

@ferhatb ferhatb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test cases ? Copy/paste is fine for initial tests. Please add a comment to keep tests in sync if updated on the group of tests.

This updates the web_ui implementation of lerpDouble to match the
behaviour of the C++ engine implementation in dart:ui.

Specifically this covers the following changes:
* #20871: stricter handling of NaN and infinity
* #20879: Improve the precision of lerpDouble

lerpDouble: stricter handling of NaN and infinity (#20871)
----------------------------------------------------------

Previously, the behaviour of lerpDouble with respect to NaN and infinity
was relatively complex and difficult to reason about. This patch
simplifies the behaviour with respect to those conditions and adds
documentation and tests.

In general, if `a == b` or both values are null, infinite, or NaN, `a`
is returned. Otherwise we require `a` and `b` and `t` to be finite or
null and the result of the linear interpolation is returned.

Improve the precision of lerpDouble (#20879)
--------------------------------------------

Reduces errors caused by the loss of floating point precision when the
two extrema of the lerp differ significantly in magnitude. Previously,
we used the calculation:

    a + (b - a) * t

When the difference in magnitude between `a` and `b` exceeds the
precision representable by double-precision floating point math, `b - a`
results in the larger-magnitude value of `a` or `b`. The error between
the value produced and the correct value is then scaled by t.

A simple example of the impact can be seen when `a` is significantly
larger in magnitude than `b`. In that case, `b - a` results in `a` and
when `t` is 1.0, the resulting value is `a - (a) * 1.0 == 0`.

The patch transforms the computation to the mathematically-equivalent
expression:

    a * (1.0 - t) + b * t

By scaling each value independently, the behaviour is more accurate.
From the point of view of performance, this adds an extra
multiplication, but multiplication is relatively cheap and the behaviour
is significantly better.

This patch also adds a `precisionErrorTolerance` constant to
test_utils.dart and migrates existing tests to use `closeTo()` for
testing.

The tests themselves *do* currently use values that have an exact
floating-point representation, but we should allow for flexibility in
future implementation changes.
@cbracken
Copy link
Member Author

cbracken commented Sep 8, 2020

Updated w/ tests added back and updated for web.

@cbracken cbracken merged commit cf8c6b8 into flutter:master Sep 8, 2020
@cbracken cbracken deleted the web-lerpDouble branch September 8, 2020 23:04
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2020
zanderso pushed a commit to flutter/flutter that referenced this pull request Sep 9, 2020
* 556cb23 Roll Skia from 6763a713f957 to d91cd6b5ee2b (3 revisions) (flutter/engine#20989)

* b6a3c54 Roll Fuchsia Linux SDK from A0PKwETay... to gfAt63Ezd... (flutter/engine#21005)

* cceb733 Roll Fuchsia Mac SDK from sih5f60Gt... to 9pfHLZEFU... (flutter/engine#21006)

* 5539820 Roll Skia from d91cd6b5ee2b to a73a84f9b8e3 (1 revision) (flutter/engine#21007)

* b4cc631 Roll Dart SDK from f3a9ca88b664 to e59935669cb0 (1 revision) (flutter/engine#21008)

* 6cf0cc4 Roll Skia from a73a84f9b8e3 to d0fe7d37d678 (1 revision) (flutter/engine#21011)

* 5b055bb Roll Skia from d0fe7d37d678 to 611a52108b01 (2 revisions) (flutter/engine#21012)

* 575a519 Enable lazy-async-stacks by-default in all modes (Take 3) (flutter/engine#20895)

* 040a794 Roll Fuchsia Mac SDK from 9pfHLZEFU... to tUwahggJ8... (flutter/engine#21013)

* 22cca4c Roll Dart SDK from e59935669cb0 to f745f9447ddf (1 revision) (flutter/engine#21014)

* 539cb69 Roll Fuchsia Linux SDK from gfAt63Ezd... to Ta3F40BV6... (flutter/engine#21015)

* 7d927dd Roll Dart SDK from f745f9447ddf to b88c06c314f4 (1 revision) (flutter/engine#21016)

* 09a5bf7 Tweak the mdns error message (flutter/engine#20991)

* d0d9ce6 Roll Fuchsia Linux SDK from Ta3F40BV6... to coVjRTWSf... (flutter/engine#21018)

* 808bb85 Roll Fuchsia Mac SDK from tUwahggJ8... to TyNHQXzNU... (flutter/engine#21019)

* 5aa6921 Roll Skia from 611a52108b01 to cd54c8385c31 (2 revisions) (flutter/engine#21021)

* e7d558f Roll Dart SDK from b88c06c314f4 to 33b6c95936e0 (2 revisions) (flutter/engine#21023)

* af90dd3 Roll Skia from cd54c8385c31 to c0d3495e1ee2 (12 revisions) (flutter/engine#21024)

* f0fb74b Avoid crashing and display error if the process cannot be prepared for JIT mode Dart VM. (flutter/engine#20980)

* 6a6c23a Roll Skia from c0d3495e1ee2 to cf1a4f50121f (6 revisions) (flutter/engine#21026)

* 716dce0 Android 30r3 (flutter/engine#21025)

* 7431070 Roll Dart SDK from 33b6c95936e0 to a2c9cae4dcd8 (1 revision) (flutter/engine#21027)

* cef383d Roll Skia from cf1a4f50121f to 04b9443274cf (2 revisions) (flutter/engine#21028)

* cf8c6b8 Update web lerpDouble to match C++ behaviour (flutter/engine#21010)

* 6866675 Roll Skia from 04b9443274cf to b8ae7fa12aa0 (1 revision) (flutter/engine#21030)

* c538f40 Roll Dart SDK from a2c9cae4dcd8 to ffbfa2000435 (1 revision) (flutter/engine#21031)

* 89d34b0 Roll Skia from b8ae7fa12aa0 to 445c8ebcb710 (1 revision) (flutter/engine#21032)

* 7766d2e Roll Fuchsia Mac SDK from TyNHQXzNU... to Phn3nF_BJ... (flutter/engine#21034)

* e3de8d0 Roll Fuchsia Linux SDK from coVjRTWSf... to eBus_y4DN... (flutter/engine#21035)

* beb7df5 Roll Skia from 445c8ebcb710 to f9d5940fef55 (3 revisions) (flutter/engine#21037)

* f3a17b6 Roll Skia from f9d5940fef55 to 81c6d6eeb4cf (1 revision) (flutter/engine#21038)

* bbcc495 Roll Dart SDK from ffbfa2000435 to 2e838b7b4503 (2 revisions) (flutter/engine#21039)

* 61062fd Roll Skia from 81c6d6eeb4cf to 81606b5d9774 (5 revisions) (flutter/engine#21041)

* f7c7b41 Revert "Enable lazy-async-stacks by-default in all modes (Take 3) (#20895)" (flutter/engine#21043)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants