-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Mesh3d lighting fix #3415
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
Mesh3d lighting fix #3415
Conversation
package.json
Outdated
@@ -77,7 +77,7 @@ | |||
"gl-heatmap2d": "^1.0.4", | |||
"gl-line3d": "^1.1.6", | |||
"gl-mat4": "^1.2.0", | |||
"gl-mesh3d": "^2.0.2", | |||
"gl-mesh3d": "git://github.com/gl-vis/gl-mesh3d.git#a8e8feaf48b280108a953f4541acbcc1f5a7aa4c", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@archmoj https://github.com/gl-vis/gl-mesh3d/compare/3353-light-reversed-ranges looks good to me. I made one small comment in gl-vis/gl-mesh3d@9fae35a, but yeah this indeed looks more similar to how That said, I did notice one odd diff in the is that new "shaded" part in the bottom third of the tetrahedra face on purpose? |
Good 👁️ |
@archmoj are you happy with the results now? Would this be a good time to take a second look at this PR? |
Yes. Now the number of changed baselines is at minimum. So, it is good to review. |
@archmoj could you explain why the green tetrahedra in appears to be all shaded now? Where is the light coming from in that mock? |
Light position is the default and the reason is that those two faces have identical angle to the light source. But if you slightly rotate the scene they would show different shadings. |
This sounds like a step in the right direction. Let's get this in 💃 |
thanks for fixing that new baseline 💃 ! |
Possibly fixes #3353 & #2477 by applying an identical light positioning and shading algorithms for
mesh3d
&surface
traces i.e. following the logic used insurface
trace.@plotly/plotly_js
Here is a demo illustrating the fix for reversed ranges.