Skip to content

Scatter3d and scattergl handling rgb colors with extra alpha values#3904

Merged
archmoj merged 8 commits into
masterfrom
fix3902-rgb-has-no-alpha
Jul 2, 2019
Merged

Scatter3d and scattergl handling rgb colors with extra alpha values#3904
archmoj merged 8 commits into
masterfrom
fix3902-rgb-has-no-alpha

Conversation

@archmoj

@archmoj archmoj commented May 24, 2019

Copy link
Copy Markdown
Contributor

Fixes #3902 .
Colors passed as rgb with an extra alpha value could cause some traces such as scatter3d & scattegl to enable opacity (as if rgba was applied).
This happens while color-normalize parses alpha for rgb similar to rgba.

Also dependencies of various gl modules including regl-splom, regl-scatter2d, regl-line2d, regl-error2d, gl-text are patched and updated in this PR.

@plotly/plotly_js

TODO:

  • Adding more tests

@archmoj archmoj added bug something broken status: in progress labels May 24, 2019
@archmoj

archmoj commented May 24, 2019

Copy link
Copy Markdown
Contributor Author

This is now reviewable excluding two reverted commits.
And here is all those patches end up:
colorjs/color-parse@master...archmoj:rgb-has-no-alpha

@etpinard

Copy link
Copy Markdown
Contributor

I think patching color-parse (in your colorjs/color-parse@master...archmoj:rgb-has-no-alpha) is the way to go, but that would probably be called a breaking change for color-parse.

You can try making a PR right away, but I suspect you'll have to make that an option in order to get it merged.

Comment thread package-lock.json
"parse-rect": "^1.2.0",
"pick-by-alias": "^1.2.0",
"point-cluster": "^3.1.4",
"point-cluster": "^3.1.5",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm. Do we really want to bump point-cluster?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It has already been applied within the master branch of regl-scatter2d: gl-vis/regl-scatter2d@cda45ae. Yet we don't have a new reg-scatter2d release.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, so can we wait until we need to bump regl-scatter2d to bump point-cluster?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. Exactly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

... unless you feel comfortable with dy/point-cluster@8419f96

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah I see, regl-scatter2d uses color-parse. Yep this package-lock diff makes perfect sense. Sorry for the confusion 👌

@etpinard

Copy link
Copy Markdown
Contributor

@archmoj will it be possible to get this PR in 1.48.0? Do you need anything else from Dima?

@archmoj

archmoj commented May 28, 2019

Copy link
Copy Markdown
Contributor Author

The only blocker is this PR: colorjs/color-normalize#4.
I suspect the reason behind the failed test could be node 12 & not color-rgba 2.1.1?

…regl-scatter2d, regl-line2d, regl-error2d and few more
@archmoj archmoj added this to the v1.49.0 milestone Jul 2, 2019
@archmoj

archmoj commented Jul 2, 2019

Copy link
Copy Markdown
Contributor Author

@etpinard finally this thing is now reviewable.

@etpinard

etpinard commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

💃 thanks!

@archmoj
archmoj merged commit 44633a1 into master Jul 2, 2019
@archmoj
archmoj deleted the fix3902-rgb-has-no-alpha branch July 2, 2019 13:31
@etpinard

etpinard commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

@archmoj Looks like this PR broke ./tasks/noci_test.sh image. On master, I get:

$ ./tasks/noci_test.sh image
renderer error - Uncaught ReferenceError: Plotly is not defined
          Chrome version 59.0.3071.115
          Electron version 1.8.4

adding --debug to

test_image () {
$root/../orca/bin/orca.js graph \
$root/test/image/mocks/mapbox_* \
--plotly $root/build/plotly.js \
--mapbox-access-token "pk.eyJ1IjoiZXRwaW5hcmQiLCJhIjoiY2luMHIzdHE0MGFxNXVubTRxczZ2YmUxaCJ9.hwWZful0U2CQxit4ItNsiQ" \
--output-dir $root/test/image/baselines/ \
--verbose || EXIT_STATE=$?
}

we get:

image

which appears to come from to-px, added in commit mikolalysenko/to-px@bd2ce00 released in to-px@1.1.0


Other info:

  • npm ls to-px gives:

image

Maybe bumping down to-px to 1.0.1 in gl-text would fix the problem @archmoj ?

@etpinard

etpinard commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

... and of course, this will be ⛔ blocking ⛔ the release of v1.49.0

@archmoj

archmoj commented Jul 2, 2019

Copy link
Copy Markdown
Contributor Author

I am on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug something broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scatter3d & Scattergl should not display alpha when having rgb colors not rgba colors

2 participants