Skip to content

Bug: graphType doesn't update correctly #329

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.

Sign up for GitHub

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

Jump to bottom
Closed
ii888ii opened this issue Nov 10, 2024 · 6 comments
Closed

Bug: graphType doesn't update correctly #329

ii888ii opened this issue Nov 10, 2024 · 6 comments
Labels
bug

Comments

@ii888ii
Copy link

ii888ii commented Nov 10, 2024 

<!DOCTYPE html>
<html lang="en">

<head>
<meta charset="utf-8" />
<script src="https://unpkg.com/function-plot/dist/function-plot.js"></script>
</head>

<body>
Does nothing:
<button id=upd>update existing fn</button>
<br>
Doesn't remove old polyline plot:
<button id=redefine>redefine data</button>
  
<div id=plot></div>
<script>
  
let obj = {
  target: plot,
  grid: true,
  data: [
    { fn: 'x' }
  ]
}
  
functionPlot(obj)

upd.addEventListener('click', evt => {
  obj.data[0].nSamples = 10
  obj.data[0].graphType = 'scatter'
  console.log(obj)
  functionPlot(obj)
})

redefine.addEventListener('click', evt => {
  obj.data = [{
    fn: 'x', nSamples: 10, graphType: 'scatter'
  }]
  console.log(obj)
  functionPlot(obj)
})

</script>
</body>

</html>
@Linho1219
Copy link
Contributor

I've met the same issue. Destroying function plot instance and rebuilding now as a temporary solution.

@stale Stale
Copy link

stale bot commented Feb 25, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Feb 25, 2025
@stale stale bot closed this as completed Apr 25, 2025
@Linho1219 Linho1219 mentioned this issue May 24, 2025
Merged
@mauriciopoppe mauriciopoppe reopened this May 24, 2025
@mauriciopoppe mauriciopoppe added bug and removed wontfix labels May 24, 2025
@mauriciopoppe
Copy link
Owner

mauriciopoppe commented May 24, 2025 •
edited
Loading

Thanks for the bug report, the initial render has a single <g class="graph"> tag

Image

The 2nd render by clicking on the first button still has a single <g class="graph"> tag however the polyline from the initial render is still there and there are <circle> tags for the scatter plot.

Image

These lines do the data join

function-plot/src/chart.ts

Lines 547 to 558 in 7596fe1

const graphs = content
.merge(contentEnter)
.selectAll(':scope > g.graph')
.data(
(d: FunctionPlotOptions) => {
return d.data
},
(d: any) => {
// The key is the function set or other value that uniquely identifies the datum.
return d.fn || d.r || d.x || d.text
}
)

There was an element already identified by the same key so the previous datum wasn't removed, it was updated. And in the update step there's the assumption that we only draw what's needed, these lines don't "undraw" or remove the first line.

function-plot/src/chart.ts

Lines 567 to 578 in 7596fe1

graphs.merge(graphsEnter).each(function (d: FunctionPlotDatum, index: number) {
// additional options needed in the graph-types/helpers
d.index = index
// (hidden property)
// @ts-ignore
d.generation = self.generation
const selection = d3Select(this)
selection.call(globals.graphTypes[d.graphType](self))
selection.call(helpers(self))
})

One way to fix this is by removing the first <g class="graph"> and recreating it again.

@mauriciopoppe
Copy link
Owner

#351 fixed this issue.

@mauriciopoppe
Copy link
Owner

Actually it didn't, I still see the bug, reopening.

@mauriciopoppe mauriciopoppe reopened this May 24, 2025
@mauriciopoppe
Copy link
Owner

After looking at the d3.selection source code it calls the key function twice:

Both refer to the same datum, so when the key function twice is called the outputs are the same, therefore for d3.selection it's an update.

What we want is to use a new datum in data[i] so that the calls look have the args:

  • key(node.__data__) = something that uniquely identifies the old datum
  • key(data[i]) = something that uniquely identifies the new datum

After #351 was merged the key function considers both fn and graphType which will help.

I think the pattern to update options.data should be on replacing each datum with a new element every time instead of updating properties of the object.

upd.addEventListener('click', evt => {
  obj.data[0] = { fn: 'x', nSamples: 10, graphType: 'scatter' }
  functionPlot(obj)
})

The above works, I'll mention this in the public docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mauriciopoppe @Linho1219 @ii888ii