Add combinePaths function & Performance improvement by balakrishna-deriv · Pull Request #245 · deriv-com/flutter-chart

balakrishna-deriv · 2023-08-15T10:35:55Z

Issue: Path.combine is not implemented in HTML mode of the flutter charts engine - flutter/flutter#44572.

In this PR, we have added a function combinePaths which is a replacement for Path.combine used in ChannelFillPainter and OscillatorLinePainter in HTML mode. combinePaths function combines and intersects two paths to generate upper and lower paths.

Added two props to control the animation duration
Added a prop to control the frames drawn per second

…nto add-web-adapter

chore: update light theme colors

…er-chart into feat-barriers-and-markers

This reverts commit 0ecba93.

This reverts commit 49fa8a8.

…nto add-web-support

…rsection.dart Co-authored-by: ramin-deriv <55975218+ramin-deriv@users.noreply.github.com>

…tter-chart into html-renderer-fix

…nto html-renderer-fix

…imation

…nto html-renderer-fix

balakrishna-deriv · 2023-10-06T09:47:35Z


    final CachedIndicator<Tick> bbmSMA =
-        MASeries.getMAIndicator(_fieldIndicator, bbOptions);
+        MASeries.getMAIndicator(_fieldIndicator, bbOptions)..calculateValues();


To fix stack overflow error when too much tick data is loaded.

@balakrishna-deriv I think we should not call calculateValues in createPainter. There are some in other Indicator series even before you guys work on it. I think those also should be removed as well
It will cause the series indicator to calculate all of its values for the entire ticks at every tick update.
The idea was to let AbstractSingleIndicatorSeries to decide when to call calculateValues and when to reuse the old result to avoid unnecessary computations.

What is the scenario which causes the stack overflow issue? for how many tick data?

@ramin-deriv Yes, calculateValues code was already there at a few places before we started working on this.

The issue happens because the calculate function is called recursively. It is reproducible on a big screen where there are more than 5,000 ticks. It happens on the dev branch too, you can try loading the app on a big landscape tablet and add indicators like ADX, SMI.

Bug video:
https://github.com/regentmarkets/flutter-chart/assets/56330681/918ce872-e07d-49d0-a20c-3fbc0e5d78de

Yes, I saw there are some in other places too. they would be unnecessary calculations and I think they also should be removed.

The thing is that AbstractSingleIndicatorSeries in the beginning calls calculateValues on the indicator. and it avoids hitting the overflow issues.

I guess some nested indicators in BollingerBands can't reuse the old indicator data and go to calculate for all and hit this issue.
But regardless I think we should find the reason ASAP since it will cause to many unnecessary calculation and will slowdown the chart when the tick history is big
I'm checking right now to see how nested indicator in BollingersBands do reuse the old result.

In addition, there can be a back-up solution by keeping track of the indices that don't have calculated result and when happens we go in a loop and calculate values in the loop and not wait for it to be calculated recursively which would have the chance of facing overflow

@ramin-deriv I get your point but not every indicator series is extending AbstractSingleIndicatorSeries. 😅
For example: ADX Series.

calculateValues in the initialize is not called for indicators like bollinger, adx, smi, etc.. because these indicators are not extending AbstractSingleIndicatorSeries.

calculateValues is called only for indicators such as cci which extends AbstractSingleIndicatorSeries.

I think it's better to do the refactor in a separate PR since we already have this pattern in the dev branch. 😄

I think If we add the initialize function to the series and call it on update it would work.
I can do it in a separate PR later as I have to test all the indicators again 😅

Yes, those that have single line of data extend AbstractSingleIndicatorSeries,
but other indicators that directly extend series are in-fact composition of multiple indicators. and on the life-cycle methods they delegate to their internal series and call the internal series methods accordingly.
and the internal series are SingleIndicatorSeries that extends the abstract one.

For sure, We can do it in another. no problem. can we create a card for it to do it in the earliest time?

balakrishna-deriv · 2023-10-06T09:54:21Z

+        if (_minElapsedTimeToFollow != null &&
+            elapsedMs < _minElapsedTimeToFollow!) {
+          return;
+        }


onNewFrame is called on every frame. On a 60fps screen, this function is getting called 60 times and it changes the rightBoundEpoch to move the xAxis.
The problem happens when we add multiple indicators to the charts. We added a prop minElapsedTimeToFollow to control the no of frames drawn per second. When there are more indicators, we draw fewer frames to make sure the chart renders smoothly.

raminvakili8 · 2023-10-09T06:38:14Z

+            .map((Tick entry) =>
+                Tick(epoch: entry.epoch, quote: _topHorizontalLine!))
+            .toList();
+        topIntersections = combinePaths(


Can we call this one time for both here and in line 127. and use for both places?

@ramin-deriv Unfortunately, we can't do that because horizontalLineEntries are different in both the places.
At line 87, horizontalLineEntries is created with _topHorizontalLine and at line 127 horizontalLineEntries is created with _bottomHorizontalLine.

raminvakili8 · 2023-10-11T03:20:56Z

    _currentTickAnimationController = AnimationController(
      vsync: this,
-      duration: const Duration(milliseconds: 300),
+      duration: _currentTickAnimationDuration,


And in the state class we just directly use widget.currentTickAnimationDuration.

this is done :)

raminvakili8 · 2023-10-11T03:42:17Z

+    if (widget.currentTickAnimationDuration != null &&
+        widget.currentTickAnimationDuration!.inMilliseconds !=
+            _currentTickAnimationDuration.inMilliseconds) {
+      _setupCurrentTickAnimation();
+    }
+
+    if (widget.quoteBoundsAnimationDuration != null &&
+        widget.quoteBoundsAnimationDuration!.inMilliseconds !=
+            _quoteBoundsAnimationDuration.inMilliseconds) {
+      _setupBoundsAnimation();
+    }


I think this part would change to this. and Duration implements comparable so I think we can just compare them without using inMillise...

if (widget.currentTickAnimationDuration != oldChart.currentTickAnimationDuration) { _setupCurrentTickAnimation(); } if (widget.quoteBoundsAnimationDuration != oldChart.quoteBoundsAnimationDuration) { _setupBoundsAnimation(); }

updated this

raminvakili8 · 2023-10-11T03:50:41Z

+    this.currentTickAnimationDuration,
+    this.quoteBoundsAnimationDuration,


I think these can set the default duration themselves. after becoming non-nullable.

Suggested change

this.currentTickAnimationDuration,

this.quoteBoundsAnimationDuration,

this.currentTickAnimationDuration = const Duration(mili,

this.quoteBoundsAnimationDuration = const Duration(mi,

updated this

raminvakili8 · 2023-10-11T03:51:16Z

+    Duration? currentTickAnimationDuration,
+    Duration? quoteBoundsAnimationDuration,


Suggested change

Duration? currentTickAnimationDuration,

Duration? quoteBoundsAnimationDuration,

Duration? currentTickAnimationDuration,

Duration? quoteBoundsAnimationDuration,

Suggested change

Duration? currentTickAnimationDuration,

Duration? quoteBoundsAnimationDuration,

super.currentTickAnimationDuration,

super.quoteBoundsAnimationDuration,

raminvakili8 · 2023-10-11T03:53:29Z

    // TODO(NA): Consider refactoring the switch with OOP pattern. https://refactoring.com/catalog/replaceConditionalWithPolymorphism.html
    switch (_currentViewingMode) {
      case ViewingMode.followCurrentTick:
+        if (_minElapsedTimeToFollow != null &&


Can we make minElapsedTimeToFollow non-nullable and with default value. so we don't have to check here.

this is done as well

balakrishna-deriv and others added 30 commits November 1, 2022 11:13

feat: expose chart scaling

e12169b

feat: web adapter init

88d5fae

chore: add models

2da772d

Merge branch 'fe-changes' of github.com:regentmarkets/flutter-chart i…

b735408

…nto add-web-adapter

chore: fix lint issues

bb0b924

chore: fix issue with map

c0a3527

chore: fix map issue 2

ff3d175

chore: update light theme colors

295d5dc

Merge pull request #2 from balakrishna-binary/update-light-theme-colors

6ef8e0e

chore: update light theme colors

feat: integrate multiplier barrier and shade

49fa8a8

Merge branch 'add-web-adapter' of github.com:balakrishna-binary/flutt…

0296765

…er-chart into feat-barriers-and-markers

feat: draggable barrier - relative and absolute

0ecba93

feat: add web markers support

e7538d3

feat: contract replay painter

322dcee

chore: add interops and callbacks

e078ece

chore: add util functions

2b0ec4c

chore: add flag to hide crosshair

67d5fcc

chore: active marker drawing and update end icon

be945fb

Revert "feat: draggable barrier - relative and absolute"

7a25fa3

This reverts commit 0ecba93.

Revert "feat: integrate multiplier barrier and shade"

f30315f

This reverts commit 49fa8a8.

chore: fix instance issue

c8646c1

fix: marker drawings

ca64964

refactor: start and end markers

e5adc32

chore: add digit contract toggle

4be4b01

refactor: tick points

6863e03

chore: fix quote grid initial render

c190fad

refactor: chart options

90db320

add indicatorsRepo

2926c20

fix: order of repaint check

c1b6b59

chore: add id to indicators

e7bd8a1

balakrishna-deriv added 6 commits August 3, 2023 14:02

chore: add line styles for rainbow indicator

c5e1146

Merge branch 'fe-changes' of github.com:regentmarkets/flutter-chart i…

779727e

…nto add-web-support

fix: merge issues

66df559

chore: add assert to RainbowIndicatorConfig

4019ef4

feat: add combinePaths function

8f0c1ae

chore: update document

df0d151

raminvakili8 approved these changes Aug 17, 2023

View reviewed changes

Comment thread lib/src/deriv_chart/chart/data_visualization/helpers/find_intersection.dart

Update lib/src/deriv_chart/chart/data_visualization/helpers/find_inte…

f833ffd

…rsection.dart Co-authored-by: ramin-deriv <55975218+ramin-deriv@users.noreply.github.com>

raminvakili8 approved these changes Aug 17, 2023

View reviewed changes

balakrishna-deriv added 9 commits August 18, 2023 18:02

Merge branch 'add-web-support' into html-renderer-fix

ca8804f

Merge branch 'html-renderer-fix' of github.com:balakrishna-binary/flu…

28f1619

…tter-chart into html-renderer-fix

Merge branch 'fe-changes' of github.com:regentmarkets/flutter-chart i…

802f471

…nto html-renderer-fix

Merge branch 'fe-changes' of github.com:regentmarkets/flutter-chart i…

4fe413d

…nto html-renderer-fix

chore: add animation dependency to marker area

ccf89c5

chore: add minElapsedTimeToFollow prop

a79a654

chore: add props to customize animation duration and to show blink an…

404f27b

…imation

Merge branch 'fe-changes' of github.com:regentmarkets/flutter-chart i…

d9c689b

…nto html-renderer-fix

chore: update distant constant to fix flickering issue

7df5515

balakrishna-deriv changed the title ~~Add combinePaths function~~ Add combinePaths function & Performance improvement Oct 6, 2023

fix: indicator exception on selecting MovingAverageType

7397c28

balakrishna-deriv commented Oct 6, 2023

View reviewed changes

Comment thread lib/src/deriv_chart/chart/basic_chart.dart

fix: duplicate import

4b60a5a

balakrishna-deriv commented Oct 6, 2023

View reviewed changes

raminvakili8 reviewed Oct 9, 2023

View reviewed changes

raminvakili8 reviewed Oct 11, 2023

View reviewed changes

refactor: make duration props non nullable

c39429d

raminvakili8 approved these changes Oct 11, 2023

View reviewed changes

fix: expanded index when bottom indicator is removed

e9b2fe9

raminvakili8 merged commit 697ca92 into deriv-com:fe-changes Oct 17, 2023

		this.currentTickAnimationDuration,
		this.quoteBoundsAnimationDuration,

		Duration? currentTickAnimationDuration,
		Duration? quoteBoundsAnimationDuration,

Uh oh!

Conversation

balakrishna-deriv commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raminvakili8 Oct 11, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

raminvakili8 Oct 11, 2023 • 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!

balakrishna-deriv Oct 11, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

raminvakili8 Oct 11, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

balakrishna-deriv commented Aug 15, 2023 •

edited

Loading

raminvakili8 Oct 11, 2023 •

edited

Loading

raminvakili8 Oct 11, 2023 •

edited

Loading

balakrishna-deriv Oct 11, 2023 •

edited

Loading

raminvakili8 Oct 11, 2023 •

edited

Loading