Fix to remove webgl warnings resulted from blank texts in scatter3d

[archmoj]

This PR resolves issue #704.
@alexcjohnson

[archmoj]

gl-vis/gl-scatter3d@master...issue-704

[alexcjohnson]

BTW now that you have PRs in these other repos, even better than the branch link would be to link to that PR directly
gl-vis/gl-scatter3d#9

[archmoj]

Thanks for the note. But the other PR on the gl-scatter3d repo is not opened by me.

[archmoj]

My apologies @alexcjohnson,
Here is the link to the open PR:
gl-vis/gl-scatter3d#9

[alexcjohnson]

Are the warnings that this PR suppresses accessible to us in a jasmine test? Or is there some other effect of the error that we can test?

[etpinard]

Are the warnings that this PR suppresses accessible to us in a jasmine test?

Unfortunately no.

Or is there some other effect of the error that we can test?

gd._fullLayout.scene._scene.glplot.objects[0].glyphBuffer.length was 0 before and is 2616 after plotting:

Plotly.newPlot(gd, [{
  type: 'scatter3d',
  mode: 'text',
  x: [1,2,3],
  y: [2,1,2],
  z: [2,1,3]
}])

@archmoj is this expected? Could we use it to 🔒 down this fix?

Also referencing https://github.com/gl-vis/gl-scatter3d/pull/9/files#r227874673

[archmoj]

Thanks @etpinard.
Yes this is really helpful.

[etpinard]

• append an it to this describe block
• call

Plotly.plot(gd, [{
  type: 'scatter3d',
  mode: 'text',
  x: [1,2,3],
  y: [2,1,2],
  z: [2,1,3]
}])

• assert that gd._fullLayout.scene._scene.glplot.objects[0].glyphBuffer.length isn't 0

• add a comment above that asserting linking to issue scatter3d: suppressing connecting lines causes WebGL warning #704

[archmoj]

The jasmine test is now able to catch the bug. Many thanks @etpinard for the helpful introduction to this test.
Next step: I need to finalize the changes to gl-scatter3d module, and I may try to fix this issue as well.
#1976

[etpinard]

Thanks for the update @archmoj

Don't worry about the ci (appveyor) failure. That appveyor container shouldn't be runned, but for some reason it does run when you pushed to a branch with a conflict.

[archmoj]

Regarding this issue and other issues I noticed in gl-scatter3d module please refer to this demo.
To test improvements and new functionalities (e.g. handling JSON data in the text) with the new version please run the JavaScript code (excluding its first line) directly in the console.

[etpinard]

Nice demo @archmoj

Have you check that those non-string items in text behave the same way in for scatter traces:

https://codepen.io/etpinard/pen/qJgPvX ?

[archmoj]

Thank you @etpinard for this demonstration. Good call if needed I could also fix the other trace in order to e.g. display JSON object properly?

[etpinard]

I could also fix the other trace in order to e.g. display JSON object properly?

No need. I don't think we should support that. We could do better than rendering [object object] but that'll be a another day.

[archmoj]

So now on scatter3d we render undefined as empty text. Sounds OK? @etpinard
Using your demo for scatter one can see they are rendered as blank which is similar to the new scatter3d behavior.
Please note that we used to render them as undefined which could have been a bit long & annoying.
Any other suggestion? @alexcjohnson

[alexcjohnson]

I'd call it a bug if undefined (or null) ever gets drawn as something other than a missing / blank value. No need to hunt around and fix other trace types here, but it's definitely a positive change for scatter3d.

[etpinard]

Sounds OK?

Looks good! Could you add JSON version of your https://codepen.io/MojtabaSamimi/pen/OBrdyE to our test mocks?

[archmoj]

Great idea @etpinard .
Yes I'll do it right now.

[archmoj]

There is a question. JSON format only has null and does not have undefined. So how we could/should handle cases where the user used undefined and of course not "undefined" (as string) there?
Should we those cases to null or "" (blank) or "undefined" (string)? I prefer not to use "undefined" as string in this scenario. Your thoughts? @etpinard @alexcjohnson

[etpinard]

There is a question. JSON format only has null and does not have undefined. So how we could/should handle cases where the user used undefined and of course not "undefined" (as string) there?
Should we those cases to null or "" (blank) or "undefined" (string)? I prefer not to use "undefined" as string in this scenario.

null, undefined, '' should all result in a blank text entry.

[archmoj]

Yes, but it is not clear to me how one could mimic the case of undefined using JSON input?
One idea could be to write nothing between two commas. Right?

[etpinard]

Pro tip @archmoj you can go ahead and close "duplicate" PRs. If something goes wrong, we can always reopen closed PRs.

[archmoj]

Thanks for the note @etpinard. OK I would follow that next time.