-
Notifications
You must be signed in to change notification settings - Fork 0
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
ChartCanvasNode and the need to call update. #45
Comments
That seems like something that could change, but I'm curious if it would affect performance. |
I unsuccessfully tried to address this in sim specific code, see phetsims/fourier-making-waves#174. |
I said above:
@jonathanolson pointed out that |
Unassigning because I doubt that we'll see progress on this for awhile. Let me know when you'd like to discuss. |
Perhaps the best way forward for this issue is to schedule a 15-minute mini dev subteam. |
Unassigning until we muster a subgroup meeting, which seems unlikely in the near future. |
Do the assignees indicate the dev group subteam? I also want to unassign myself since I don't want to see this in my assigned issues until our subgroup meeting. But leaving myself as an assignee helps indicate that I'm part of the subgroup when it's time. |
Unassigning since I am not scheduled any time for this part of bamboo in the current quarter. This issue will not be lost since I am the responsible dev for the repo. |
For #47 and phetsims/fourier-making-waves#165 ...
@jonathanolson added this
REVIEW
comment to Foruier's SumChartNode.js:This is a problem that @samreid and I wrestled with previously in bamboo.
ChartCanvasNode
renders 1 or moreCanvasLinePlot
instances.CanvasLinePlot
does not know aboutChartCanvasNode
. So when you make changes to aCanvasLinePlot
, you are currently responsible for callingChartCanvasNode.update
.This is well-documented in
CanvasLinePlot
, for example:Despite the documentation, this is still a pain point - easy to forget, and (as @jonathanolson pointed out in the REVIEW comment) a common pattern that results in duplicated code.
But I should also point out... In terms of performance, there are also benefits here. In Fourier, I have a ChartCanvasNode that is rendering 97 CanvasLinePlot instances. I can update all 97 plots, then call
update
. If ChartCanvasNode updated each time one of its plots was updated, then I'd have big problems in Fourier. So perhaps having to callupdate
is OK for something like this where performance is critical.I don't plan to address this for Fourier because of the need to move forward. But let's discuss this again, adding @jonathanolson to the discussion.
The text was updated successfully, but these errors were encountered: