Mesh3d cell data checks #3369
Mesh3d cell data checks #3369
Conversation
archmoj
changed the title
| if(indices) { | ||
| // otherwise, convert all face indices to ints | ||
| indices.forEach(function(index) { | ||
| for(var i = 0; i < index.length; ++i) index[i] |= 0; |
There was a problem hiding this comment.
Hmm. So you didn't move this to mesh3d/convert.js? Just 🔪 ing it didn't break any tests?
Can you try plotting a mesh3d trace with non-integer indices and see what happens?
There was a problem hiding this comment.
@etpinard Thanks for the review.
That's true! One could get bad errors in case of having non-integer ids.
So, in the revised code I added map functions to round the float values to integers.
However; there is now extra logic to check and handle for such cases. A series of jasmine tests are also added to define the expected behaviour in various scenarios.
archmoj
changed the title
src/traces/mesh3d/convert.js
Outdated
| !hasValidIndices(data.k, numVertices) || | ||
| !isTriangle([data.i, data.j, data.k]) | ||
| ) { | ||
| data.visible = false; |
There was a problem hiding this comment.
We can't set visible: false here. fullData keys can't be mutated after the defaults step (unless they are _ properties) for our Plotly.react diffing to work correctly.
Just removing this line here will suffice (i.e. and keeping the early return below) should suffice.
There was a problem hiding this comment.
Thanks for the info on this. Simply removed the line. And changed jasmine tests for the trace to be visible in all those cases.
| type: 'mesh3d' | ||
| }]) | ||
| .then(function() { | ||
| assertVisibility(true, 'to be visible'); |
There was a problem hiding this comment.
Right, this is correct: fullData will have visible:false. But this doesn't test any of the new logic in convert.js.
Could you add a check for e.g.
gd._fullLayout.scene._scene.glplot.objects[0].positions
which should be an ok proxy.
There was a problem hiding this comment.
Thanks, sorry for not catching this sooner.
There was a problem hiding this comment.
No problem. Previously we get different visibility values there...
| return b; | ||
| } | ||
|
|
||
| // Unpack position data |
There was a problem hiding this comment.
@etpinard wondering where could these functions go if we want to reuse these functions in other places e.g. in iso-surfaces?
There was a problem hiding this comment.
I'd vote for putting helper functions like this one in a new file src/traces/mesh3d/helpers.js.
test/jasmine/tests/mesh3d_test.js
Outdated
| .catch(failTest) | ||
| .then(done); | ||
| function assertPositions(exp, msg) { | ||
| expect(gd._fullLayout.scene._scene.glplot.objects[0].positions.length !== undefined).toBe(exp, msg); |
There was a problem hiding this comment.
Can we test the actual length values, not just whether positions and cells are arrays like you did in your latest commit?
There was a problem hiding this comment.
Good call. I'll fix that.
src/traces/mesh3d/defaults.js
Outdated
| }); | ||
| // three indices should be all provided or not | ||
| if( | ||
| (traceIn.i && (!traceIn.j || !traceIn.k)) || |
There was a problem hiding this comment.
Ha, you're using traceIn here. So something like
{
i: 1,
j: true,
k: {}
}
would have visible:true.
Could we be more stringent and use traceOut instead, to ensure that i,j,k are all non-empty arrays.
There was a problem hiding this comment.
Good Call. Now traceOut is applied.
Noting that visible would "only" be false in case of bad 'vertices'.
There was a problem hiding this comment.
Hmm. I don't understand traceOut.i, traceOut.j and traceOut.k should be defined at this stage. They get filled in during the coerce() calls in readComponents below.
There was a problem hiding this comment.
You are right. That is fixed in 29f5a06
Now the traces become invisible in case of bad index arrays.
|
All right, good enough for now 💃 Thanks! |
Fix #2630 by removing the indices clean up from the
defaultscode.@plotly/plotly_js