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

Rescale timeline-chart post zoom using PIXI.Container scale property #246

Merged

Conversation

hoangphamEclipse
Copy link
Contributor

This commit uses PIXI.Container scale property to rescale the chart after each zoom in/zoom out to instead of the using the manually repositioning each elements in the chart to improve the performance of the timeline chart. The scale of the time-graph-chart is changed everytime the view range changes, and is reset once the server has replied to the front-end.

This commit removes the method updateScaleAndPosition(), which was used to manually rescaling the chart.

There are 3 use cases that are affected by this change:

[1] Zooming in and out using the mouse wheel
[2] Zooming in using the selection tool (right-click and drag)
[3] Zooming in using the action buttons (+/-) on the top right of the chart.

Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. I started with testing this change, I noticed that the marker axis is not scaled properly when re-sizing the view. In the video below, the lost event markers are not scaled as the states in the main window.

rescale-issue1

Could you please look into this?

@hoangphamEclipse hoangphamEclipse marked this pull request as draft April 25, 2023 15:08
@hoangphamEclipse hoangphamEclipse marked this pull request as ready for review April 25, 2023 18:33
@hoangphamEclipse
Copy link
Contributor Author

hoangphamEclipse commented Apr 27, 2023

Pushed a fix for a bug that occurs when I close the timeline chart and reopen it, the zooming and panning stop working.

Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I get some exceptions in the console log.

  1. When I vertically resize I get the following exception:
aphics.ts:934 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'clear')
    at Graphics.clear (Graphics.ts:934:24)
    at TimeGraphComponent.clear (time-graph-component.ts:68:29)
    at TimeGraphComponent.update (time-graph-component.ts:79:14)
    at time-graph-chart.ts:934:26
    at Map.forEach (<anonymous>)
    at TimeGraphChart._this.ensureRowLinesFitViewWidth (time-graph-chart.ts:933:28)
    at _zoomRangeChangedHandler (time-graph-chart.ts:356:18)
    at time-graph-state-controller.ts:48:53
    at Array.forEach (<anonymous>)
    at TimeGraphStateController.handleZoomChange (time-graph-state-controller.ts:48:34)
G
  1. When I enable a marker set, then I get multiple exceptions.
act-dom.development.js:22839 Uncaught TypeError: Cannot read properties of null (reading 'clear')
    at Graphics.clear (Graphics.ts:934:24)
    at TimeGraphComponent.clear (time-graph-component.ts:68:29)
    at TimeGraphComponent.update (time-graph-component.ts:79:14)
    at time-graph-chart.ts:934:26
    at Map.forEach (<anonymous>)
    at TimeGraphChart._this.ensureRowLinesFitViewWidth (time-graph-chart.ts:933:28)
    at push.../../../../timeline-chart/timeline-chart/lib/layer/time-graph-chart.js.TimeGraphChart.update (time-graph-chart.ts:374:14)
    at time-graph-container.ts:110:36
    at Array.forEach (<anonymous>)
    at TimeGraphContainer.updateCanvas (time-graph-container.ts:110:21)
G
splayObject.ts:657 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'position')
    at Container.get (DisplayObject.ts:657:31)
    at TimeGraphViewportLayer._this.shiftStage (time-graph-viewport-layer.ts:48:24)
    at time-graph-state-controller.ts:51:57
    at Array.forEach (<anonymous>)
    at TimeGraphStateController.handlePositionChange (time-graph-state-controller.ts:51:38)
    at TimeGraphStateController.set (time-graph-state-controller.ts:150:14)
    at TimeGraphContainer.calculatePositionOffset (time-graph-container.ts:134:44)
    at time-graph-state-controller.ts:158:56
    at Array.forEach (<anonymous>)
    at TimeGraphStateController.handleOnWorldRender (time-graph-state-controller.ts:158:37)
g
aphics.ts:934 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'clear')
    at Graphics.clear (Graphics.ts:934:24)
    at TimeGraphComponent.clear (time-graph-component.ts:68:29)
    at TimeGraphComponent.update (time-graph-component.ts:79:14)
    at push.../../../../timeline-chart/timeline-chart/lib/components/time-graph-axis-scale.js.TimeGraphAxisScale.update (time-graph-axis-scale.ts:132:21)
    at push.../../../../timeline-chart/timeline-chart/lib/components/time-graph-grid.js.TimeGraphGrid.update (time-graph-grid.ts:26:21)
    at push.../../../../timeline-chart/timeline-chart/lib/layer/time-graph-chart-grid.js.TimeGraphChartGrid.update (time-graph-chart-grid.ts:29:28)
    at _updateHandler (time-graph-chart-grid.ts:23:48)
    at time-graph-state-controller.ts:51:57
    at Array.forEach (<anonymous>)
    at TimeGraphStateController.handlePositionChange (time-graph-state-controller.ts:51:38)
G

Could you please look into those exceptions?

hoangphamEclipse added a commit to hoangphamEclipse/theia-trace-extension that referenced this pull request May 5, 2023
The performance improvement patch for the timeline chart [1] changes
the parameters provided to handlers of the onViewRangeChanged event
on the unit controller. This commit updates the Theia Trace Extension
to ensure that it works with the performance improvement patch.

[1]: eclipse-cdt-cloud/timeline-chart#246

Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>
hoangphamEclipse added a commit to hoangphamEclipse/theia-trace-extension that referenced this pull request May 5, 2023
The performance improvement patch for the timeline chart [1] changes
the parameters provided to handlers of the onViewRangeChanged event
on the unit controller. This commit updates the Theia Trace Extension
to ensure that it works with the performance improvement patch.

[1]: eclipse-cdt-cloud/timeline-chart#246

Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>
@hoangphamEclipse hoangphamEclipse requested a review from bhufmann May 8, 2023 15:02
@bhufmann bhufmann requested review from PatrickTasse and bhufmann and removed request for bhufmann and williamsyang-work May 10, 2023 15:10
@PatrickTasse
Copy link
Contributor

After a zoom in/out, the arrows are temporarily not positioned properly until some refresh fixes them shortly later after a debounce.

Furthermore, sometimes, that late refresh never happens and the arrows are left at the wrong position. I believe this is when zooming out and the existing model with its extra range before and after still covers the new window range and the 'maybeFetchNewData' does not trigger a fetch.

The text labels on states are stretched or squeezed before the refresh, although this is not as critical as the arrow positions. And the text label can also get stuck in that stretched or squeezed state when the refresh does not happen.

@hoangphamEclipse
Copy link
Contributor Author

hoangphamEclipse commented May 11, 2023

After a zoom in/out, the arrows are temporarily not positioned properly until some refresh fixes them shortly later after a debounce.

Furthermore, sometimes, that late refresh never happens and the arrows are left at the wrong position. I believe this is when zooming out and the existing model with its extra range before and after still covers the new window range and the 'maybeFetchNewData' does not trigger a fetch.

I was able to reproduce this issue. I will fix it.

The text labels on states are stretched or squeezed before the refresh, although this is not as critical as the arrow positions. And the text label can also get stuck in that stretched or squeezed state when the refresh does not happen.

Unfortunately, it is a drawback of scaling the chart layer: everything in the chart is scaled to the same ratio. I will scale the text to make sure it stays legible/same size no matter how zoomed in the chart is.

@hoangphamEclipse
Copy link
Contributor Author

Added documentation on the fix.

This commit uses PIXI.Container scale property to rescale the chart after
each zoom in/zoom out to instead of the using the manually repositioning
each elements in the chart to improve the performance of the timeline chart.
The scale of the time-graph-chart is changed everytime the view range
changes, and is reset once the server has replied to the front-end.

This commit removes the method updateScaleAndPosition(), which was used
to manually rescaling the chart.

Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks a lot for the performance improvement.

@hoangphamEclipse hoangphamEclipse merged commit 08fb2e6 into eclipse-cdt-cloud:master May 24, 2023
@hoangphamEclipse hoangphamEclipse deleted the scale-improvement branch May 24, 2023 20:05
hoangphamEclipse added a commit to hoangphamEclipse/theia-trace-extension that referenced this pull request May 24, 2023
The performance improvement patch for the timeline chart [1] changes
the parameters provided to handlers of the onViewRangeChanged event
on the unit controller. This commit updates the Theia Trace Extension
to ensure that it works with the performance improvement patch.

[1]: eclipse-cdt-cloud/timeline-chart#246

Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>
hoangphamEclipse added a commit to hoangphamEclipse/theia-trace-extension that referenced this pull request May 24, 2023
The performance improvement patch for the timeline chart [1] changes
the parameters provided to handlers of the onViewRangeChanged event
on the unit controller. This commit updates the Theia Trace Extension
to ensure that it works with the performance improvement patch.

[1]: eclipse-cdt-cloud/timeline-chart#246

Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>
hoangphamEclipse added a commit to eclipse-cdt-cloud/theia-trace-extension that referenced this pull request May 25, 2023
The performance improvement patch for the timeline chart [1] changes
the parameters provided to handlers of the onViewRangeChanged event
on the unit controller. This commit updates the Theia Trace Extension
to ensure that it works with the performance improvement patch.

[1]: eclipse-cdt-cloud/timeline-chart#246

Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>
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.

4 participants