Fix to enable switching modes using plotly react method on scattergl traces

[archmoj]

This PR resolves the issue #2999 by providing additional checks before calling destroy functions of scattergl. Without these checks in scenarios namely switching back and forth between different modes e.g. markers to lines and then back to markers the program (depending on the device/browser) may complain and/or stop.
@alexcjohnson

[etpinard]

Without these checks in scenarios namely switching back and forth between different modes e.g. markers to lines and then back to markers the program (depending on the device/browser) may complain and/or stop.

Were you able to replicate the problem? I'm no opposed to this solution, but I'd like to understand why this happens.

[archmoj]

@etpinard Could you please try this demo?
The graph may stop updates after few switch modes.

[etpinard]

Hmm. I can't replicate.

Do you have an idea why this happens? It smells like a race condition, but I'd like to confirm.

[archmoj]

We used to have similar checks before calling destroy in traces/splom/index.js
It could be a race condition. Or the the object key is not completely deleted.
After this fix I still get some warnings on FireFox after revision 6:
WebGL warning: Exceeded 16 live WebGL contexts for this principal, losing the least recently used one.

[etpinard]

We used to have similar checks before calling destroy in traces/splom/index.js

Hmm I see. Referencing:

plotly.js/src/traces/splom/index.js

Lines 155 to 157 in 7ae6fd6

	if(scene.matrix && scene.matrix.destroy) {
	scene.matrix.destroy();
	}

I put those lines in. I think they were intended to fix intermittent failures on CI.

Note that removing that && scene.matrix.destroy does not make any test fail locally on my machine. Maybe someone could try to double-check?

---

All in all, I'll like to get to the bottom of this issue. It doesn't have to be now though. A lot of users have commented on #2999, so maybe we should try to merge this fix in before 1.42.0 and take a deeper dive later.

[etpinard]

Non-blocking comment: We (me and @alexcjohnson ) would usually write these lines as

if(scene.fill2d && scene.fill2d.destroy) {
    scene.fill2d.destroy();
}

[etpinard]

and this thing as

scene.glText.forEach(function(text) {
    if(text.destroy) text.destroy();
});

[etpinard]

Since I (and presumably other people) can't replicate the problem, it might be hard to write a jasmine test to lock this down.

[archmoj]

Interesting observation: Using the code identical to the one presented in the codepen on Chrome the scene.destroy method is not called. But on FireFox scene.destroy is called. Could you point me to the where the destroy method is triggered?

[etpinard]

Could you point me to the where the destroy method is triggered?

here for scattergl ->

plotly.js/src/plots/cartesian/index.js

Lines 315 to 320 in 7ae6fd6

	if(hadGl && !hasGl) {
	for(k in oldPlots) {
	plotinfo = oldPlots[k];
	if(plotinfo._scene) plotinfo._scene.destroy();
	}
	}

here for splom ->

plotly.js/src/traces/splom/base_plot.js

Lines 177 to 186 in 7ae6fd6

	for(i = 0; i < oldFullData.length; i++) {
	var oldTrace = oldFullData[i];
	if(!lookup[oldTrace.uid]) {
	var scene = oldFullLayout._splomScenes[oldTrace.uid];
	if(scene && scene.destroy) scene.destroy();
	// must first set scene to null in order to get garbage collected
	oldFullLayout._splomScenes[oldTrace.uid] = null;
	delete oldFullLayout._splomScenes[oldTrace.uid];
	}
	}

and similar logic will be added for scatterpolargl in #3098

[archmoj]

This should fix the issue for switching modes.
But we may need to open a new PR for the issue of switching line types namely 'scattergl' & 'scatter' where on FF we could get warning messages like this one: WebGL warning: Exceeded 16 live WebGL contexts for this principal, losing the least recently used one.
I have to say the probability on getting those warning is now significantly reduced by the last commit.

[etpinard]

Thanks for the update @archmoj !

It would be nice to investigate (and hopefully fix 😄 ) that FF too-many-webgl-context bug this afternoon. I can help out.

[archmoj]

Thanks @etpinard for the help on this.
I think we could merge this branch.
And then regarding the warning messages we create another branch & PR to resolve this PHX issue.
What do you think?

[etpinard]

I think we could merge this branch.

We'll need to add a test before merging.

And then regarding the warning messages we create another branch

Creating another branch is fine 👌

[etpinard]

I can help you add that test if you like.

[archmoj]

Great! Any help on tests would be much appreciated.