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.

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

Unlock gl-shader #2293

Merged
merged 3 commits into from
Jan 26, 2018
Merged

Unlock gl-shader #2293

merged 3 commits into from
Jan 26, 2018

Conversation

dy
Copy link
Contributor

@dy dy commented Jan 25, 2018

@dy dy mentioned this pull request Jan 25, 2018
4 tasks
@dy
Copy link
Contributor Author

dy commented Jan 25, 2018

@alexcjohnson could you please have a look :)

@dy
Copy link
Contributor Author

dy commented Jan 25, 2018

@nicolaskruchten can you please try bundling w/yarn the git@github.com:plotly/plotly.js.git#7b1bb6a5aa9fae0d4d05420f7ae81798f4fc5ef8? That should fix the issue.

package.json Outdated
@@ -69,18 +69,19 @@
"es6-promise": "^3.0.2",
"fast-isnumeric": "^1.1.1",
"font-atlas-sdf": "^1.3.3",
"gl-axes3d": "^1.2.6",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we adding a package but making no direct reference to it?

Copy link
Contributor Author

@dy dy Jan 25, 2018

Choose a reason for hiding this comment

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

@alexcjohnson good point. I wonder if we should remove gl-shader dependency as well, since we're not using it by plotly directly. @etpinard had it fixed version at the root level to ensure that all components use the same gl-shader. But that is no more relevant since now on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds reasonable to me, but after the mess we had yesterday with npm ls I'd like to figure this out with @etpinard 's input.

@alexcjohnson
Copy link
Collaborator

💃 from my side, but I'd like to hear from @nicolaskruchten whether this accomplished the goal.

@nicolaskruchten
Copy link
Contributor

💃 from me, I tested all permutations of installer=(yarn, npm) and bundler=(browserify, webpack) and the WebGL bug doesn't appear :)

@dy dy merged commit cceff86 into master Jan 26, 2018
@dy dy deleted the unlock-gl-shader branch January 26, 2018 17:07
@etpinard
Copy link
Contributor

etpinard commented Jan 29, 2018

This PR seems to have made the tests fail on master

https://circleci.com/gh/plotly/plotly.js/6120

@etpinard
Copy link
Contributor

Re-running the tests on master w/o cache (i.e. with a brand new node_modules/ folder) helps a little bit:

https://circleci.com/gh/plotly/plotly.js/6129

Only 3 surface mocks are failing:

image

@etpinard
Copy link
Contributor

etpinard commented Jan 29, 2018

After re-running the tests w/o cache off this PR's last commit 0d1d19b, the same surface mocks are failing.

https://circleci.com/gh/plotly/plotly.js/6130?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

This is good news. At least we have some sort of consistency.

@dy dy mentioned this pull request Jan 29, 2018
@dy
Copy link
Contributor Author

dy commented Jan 29, 2018

@etpinard added baselines 8456f95, seems there is very subtle shading or dithering difference, as if color conversion is a bit wrong. I'll try to reproduce that.

@dy
Copy link
Contributor Author

dy commented Jan 29, 2018

@etpinard tbh that is pretty mystical difference. That is not even clear if the behavior before was correct. That seems like internal WebGL thing − as if order of binding affects attribute/uniform precision here or in a place like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants