Skip to content
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

Bug fix: applying camera up.z vector at scene init #3256

Merged
merged 23 commits into from
Nov 22, 2018
Merged

Bug fix: applying camera up.z vector at scene init #3256

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f82007b
fix for issue 2425
archmoj Nov 17, 2018
d8ef9eb
revised camera positions in 3 mocks
archmoj Nov 17, 2018
556268b
updated mocks and baseline images
archmoj Nov 17, 2018
eb72856
moved overwrites to layout defaults
archmoj Nov 17, 2018
789cfd9
fix syntax
archmoj Nov 17, 2018
4014c35
revised snowden mock and baseline
archmoj Nov 17, 2018
1ae12b3
revert snowden mock
archmoj Nov 17, 2018
cb6dab7
revert gl3d_surface-lighting mock to better display the old bug
archmoj Nov 18, 2018
67ea429
improved logic
archmoj Nov 18, 2018
5c3742c
minimal fix
archmoj Nov 18, 2018
3ab1b1d
changed defaults and baseline
archmoj Nov 18, 2018
5b24f51
logic to enable turntable default
archmoj Nov 19, 2018
d338058
revert gl3d layout test
archmoj Nov 19, 2018
b8da9ba
revert baselines
archmoj Nov 19, 2018
899b568
changes to gl3d layout test
archmoj Nov 19, 2018
724416b
fixes for jasmine test
archmoj Nov 19, 2018
c9eed54
fix for restyle calls
archmoj Nov 19, 2018
b65fa61
adding jasmine tests for camera init
archmoj Nov 20, 2018
d25c335
added camera tests
archmoj Nov 20, 2018
f8bf472
revised auto logic i.e. without touching layout inputs
archmoj Nov 20, 2018
6af04f2
untouch gl3d layout test
archmoj Nov 20, 2018
9ef8a70
forced graphs using deprecated cameraposition attribute to stay upwards
archmoj Nov 21, 2018
f43f944
reverted and changed baseline that uses cameraposition
archmoj Nov 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/plots/gl3d/layout/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,34 @@ function handleGl3dDefaults(sceneLayoutIn, sceneLayoutOut, coerce, opts) {
sceneLayoutIn.aspectmode = sceneLayoutOut.aspectmode;
}

function enableTurnTable() {
sceneLayoutIn.dragmode = 'turntable';
}

if(sceneLayoutIn.dragmode !== false) {
if(!sceneLayoutIn.dragmode) {
// Because the default is now set to orbit mode
// (i.e. in order to apply camera.z.up at init time)
// we set turnable our disarable option here.

if(sceneLayoutIn.camera &&
sceneLayoutIn.camera.up) {

var x = sceneLayoutIn.camera.up.x;
var y = sceneLayoutIn.camera.up.y;
var z = sceneLayoutIn.camera.up.z;

if(!x || !y || !z) {
enableTurnTable();
} else if(z / Math.sqrt(x * x + y * y + z * z) > 0.999) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR is getting better by the minute. Thanks for your efforts @archmoj

Now, would there be a way to tweak this condition such that gl3d_ibm-plot and gl3d_surface-lighting still default to dragmode: 'turntable'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point @etpinard. Let me check.

enableTurnTable();
}
} else {
enableTurnTable();
}
}
}

supplyGl3dAxisLayoutDefaults(sceneLayoutIn, sceneLayoutOut, {
font: opts.font,
scene: opts.id,
Expand Down
2 changes: 1 addition & 1 deletion src/plots/gl3d/layout/layout_attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ module.exports = {
valType: 'enumerated',
role: 'info',
values: ['orbit', 'turntable', 'zoom', 'pan', false],
dflt: 'turntable',
dflt: 'orbit',
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I would prefer removing the dflt key entirely - as the default value is now dynamic.

Could you also add some info in the description about the new orbit-vs-turntable logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the description. Thanks @etpinard for the note. I will give removing dflt a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once I tried to remove dragmode.dflt, the default behaviour for drag using right-click (orbit/turntable) is not activated.
So we may keep it there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to pass the dynamic default to dragmode here:

plotly.js/src/plots/gl3d/layout/defaults.js

Line 141 in c9eed54

coerce('dragmode', opts.getDfltFromLayout('dragmode'));

getDfltFromLayout is part of it but then there's the dynamic default based on camera. I don't think you want to be altering sceneIn.dragmode with enableTurnTable, you just want to be setting the dynamic default. The logic is something like:

  • If getDfltFromLayout returns a value, use that.
  • If not but the user provides a camera that's not {x: 0, y: 0, z: >0} dfltDragmode='orbit'
  • If neither of those, dfltDragmode = 'turntable'
  • Then you call coerce('dragmode', dfltDragmode) so the user can still explicitly provide whatever.
  • The only case that's still weird at that point is if the user explicitly says dragmode: 'turntable', camera: {up: {not z-up}}. My inclination at that point is to set sceneOut.camera.up (and NOT sceneIn.camera.up) to {x: 0, y: 0, z: 1}. That won't change the behavior - the user might still wonder why up is not being honored - but at least it would raise an alarm if you call Plotly.validate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alexcjohnson, I will modify it to be this way.

editType: 'plot',
description: [
'Determines the mode of drag interactions for this scene.'
Expand Down
Binary file modified test/image/baselines/gl3d_ibm-plot.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/gl3d_surface-lighting.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion test/jasmine/tests/gl3d_plot_interact_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ describe('Test gl3d plots', function() {

it('@gl should be able to reversibly change trace type', function(done) {
var _mock = Lib.extendDeep({}, mock2);
var sceneLayout = { aspectratio: { x: 1, y: 1, z: 1 } };
var sceneLayout = { aspectratio: { x: 1, y: 1, z: 1 }, dragmode: 'turntable' };
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you need to change this? I'm fearing a breaking change here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to set turnable the default behaviour the function needs to add the dragmode attribute to the sceneLayout.

Copy link
Contributor

Choose a reason for hiding this comment

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

So without setting dragmode: 'turntable' here, it would default to 'orbit'?

What are the camera settings in this case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@etpinard notice this isn't a setting, it's an expectation. But I agree, we shouldn't be mutating the input scene, per my comment above


Plotly.plot(gd, _mock)
.then(delay(20))
Expand Down
7 changes: 4 additions & 3 deletions test/jasmine/tests/gl3dlayout_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ describe('Test Gl3d layout defaults', function() {
layoutIn = { scene: {}, dragmode: 'orbit' };
supplyLayoutDefaults(layoutIn, layoutOut, fullData);
expect(layoutOut.scene.dragmode)
.toBe('orbit', 'to user layout val if valid and 3d only');
.toBe('turntable', 'to turntable if valid and 3d only');

layoutIn = { scene: {}, dragmode: 'invalid' };
supplyLayoutDefaults(layoutIn, layoutOut, fullData);
Expand Down Expand Up @@ -230,14 +230,15 @@ describe('Test Gl3d layout defaults', function() {
.toBe('closest', 'to default if not valid');
});

it('should add data-only scenes into layoutIn', function() {
it('should add data-only scenes into layoutIn and also enable turntable', function() {
layoutIn = {};
fullData = [{ type: 'scatter3d', scene: 'scene' }];

supplyLayoutDefaults(layoutIn, layoutOut, fullData);
expect(layoutIn.scene).toEqual({
aspectratio: { x: 1, y: 1, z: 1 },
aspectmode: 'auto'
aspectmode: 'auto',
dragmode: 'turntable'
});
});

Expand Down