Skip to content

Regression bug fix to draw 3d traces after heatmap #3360

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

Merged
merged 7 commits into from
Dec 21, 2018

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Dec 20, 2018

Fixes #3359 by not calling autoType function on z axis for 3d traces after axis type was to multicategory by heatmap trace.
@etpinard

@archmoj archmoj added bug something broken regression this used to work labels Dec 20, 2018
@@ -91,6 +91,7 @@ function setAutoType(ax, data) {
}
}
else {
if(axLetter === 'z') opts.noMultiCategory = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

That works. But opts.noMultiCategory should already be true from

var opts = {noMultiCategory: !traceIs(d0, 'cartesian') || traceIs(d0, 'noMultiCategory')};

I suspect d0 in this scope is fullData[0] when it should be the first trace on the gl3d subplot.

I hope that makes sense 😄

Copy link
Contributor Author

@archmoj archmoj Dec 21, 2018

Choose a reason for hiding this comment

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

You are right. It seems to be the first trace on fullData which is not invisible.
The issue is not only affected 3d traces. The example below also fails on 'latest':

Plotly.newPlot(gd, {
    "data": [
      {
        "type": "heatmap",
        "x": [0, 1, 2],
        "y": [0, 1, 2],
        "z": [
          [0, 1, 0],
          [1, 0, 1],
          [0, 1, 0]
        ]
      },
      {
        "type": "scattergl",
        "x": [0, 1, 2],
        "y": [0, 1, 2]
      }
    ],
    "layout": {
        "title": "scattergl plot on top of 2d heatmap",
        "width": 600,
        "height": 400
    }
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard The 2d case was mentioned above failed on v.1.43.0; but works on master. So I would fetch master changes and fix this for the 3D case as you mentioned.

supplyGl3dAxisLayoutDefaults(sceneLayoutIn, sceneLayoutOut, {
font: opts.font,
scene: opts.id,
data: opts.fullData,
data: fullGl3dData,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done, but let's ♻️

/**
* Get the data trace(s) associated with a given subplot.
*
* @param {array} data plotly full data array.
* @param {string} type subplot type to look for.
* @param {string} subplotId subplot id to look for.
*
* @return {array} list of trace objects.
*
*/
exports.getSubplotData = function getSubplotData(data, type, subplotId) {
if(!Registry.subplotsRegistry[type]) return [];
var attr = Registry.subplotsRegistry[type].attr;
var subplotData = [];
var trace, subplotX, subplotY;
if(type === 'gl2d') {
var spmatch = subplotId.match(SUBPLOT_PATTERN);
subplotX = 'x' + spmatch[1];
subplotY = 'y' + spmatch[2];
}
for(var i = 0; i < data.length; i++) {
trace = data[i];
if(type === 'gl2d' && Registry.traceIs(trace, 'gl2d')) {
if(trace[attr[0]] === subplotX && trace[attr[1]] === subplotY) {
subplotData.push(trace);
}
}
else {
if(trace[attr] === subplotId) subplotData.push(trace);
}
}
return subplotData;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard The try to reuse the getSubPlotData function was not successful.
Could I revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many thanks @etpinard for the help figuring this out!
Now the tests are all passed.

@etpinard
Copy link
Contributor

Nice fix 💃 !

@archmoj archmoj merged commit 874ac4c into master Dec 21, 2018
@archmoj archmoj deleted the dont-call-autotype-for-3d branch December 21, 2018 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken regression this used to work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants