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

Surfaces are too dark #25

Closed
ChristopherChudzicki opened this issue Apr 8, 2022 · 7 comments
Closed

Surfaces are too dark #25

ChristopherChudzicki opened this issue Apr 8, 2022 · 7 comments

Comments

@ChristopherChudzicki
Copy link
Collaborator

ChristopherChudzicki commented Apr 8, 2022

Left: v0.0.5 / Right: v2+

Screen Shot 2022-04-07 at 8 30 26 PM

@sritchie I dunno if this is a consequence of changes we've made in Mathbox, or a consequence of the ThreeJS upgrades. Do you have any idea? I think you've looked at more versions of ThreeJS than I have. Otherwise... I'll try to learn something about ThreeJS :)

@sritchie
Copy link
Collaborator

sritchie commented Apr 8, 2022

Interesting, I only remember the second one through all my testing. I will look!

@sritchie
Copy link
Collaborator

sritchie commented Apr 8, 2022

It looks like it was dark at 98c3740, my first big merge (check out and run npm install && gulp build). So I must have introduced some problem during the migration from Coffeescript...

@sritchie
Copy link
Collaborator

sritchie commented Apr 8, 2022

If it helps to debug, it also happens in:

file:///Users/sritchie/code/js/mathbox/examples/test/vertexcolor.html
file:///Users/sritchie/code/js/mathbox/examples/test/transition.html
file:///Users/sritchie/code/js/mathbox/examples/test/surface.html
file:///Users/sritchie/code/js/mathbox/examples/test/subdivide.html
file:///Users/sritchie/code/js/mathbox/examples/test/repeat.html
file:///Users/sritchie/code/js/mathbox/examples/math/procedural.html
file:///Users/sritchie/code/js/mathbox/examples/math/hopf.html
file:///Users/sritchie/code/js/mathbox/examples/demo/shapes.html
file:///Users/sritchie/code/js/mathbox/examples/demo/cylindrical-stream.html

It does NOT seem to happen in
file:///Users/sritchie/code/js/mathbox/examples/test/fragmentcolor.html
file:///Users/sritchie/code/js/mathbox/examples/demo/cylindrical.html

which seemed surprising.

@ChristopherChudzicki
Copy link
Collaborator Author

ChristopherChudzicki commented Apr 9, 2022

  • Left: Steven's original at eaeb8e1 using ThreeJS v71
  • Middle: Znah znah@593bdab using ThreeJS v84
  • Right: This current repo, with ThreeJS v137

Whatever happened, I'm fairly certain it is not related to Shadergraph. I dropped an old version of Shadergraph into the current repo and stuff was still dark. I also plugged in an old v0.05 version of shaders.js (recall it used to get built to build/shaders.js) and things were still dark.

Znah's is not as dark as modern, but it does have dark lines. Steven's original looks best :/ Hope we can get back to that.

Screen Shot 2022-04-08 at 11 18 32 PM

EDIT: I think there are two separate issues: Shading and line color. If you turn off the lines, znah and 0.0.5 look the same.

@sritchie
Copy link
Collaborator

sritchie commented Apr 9, 2022

@ChristopherChudzicki I agree. here are some clues:

  • change "shaded" to false on current master and you'll get the nice luminous colors again, BUT with dark lines and no shading:

image

  • On 0.0.5, setting lineX or lineY to true vs false has no effect. the lines are missing in both cases. THAT feels like a bug to me in 0.0.5.

So that makes me suspect something in https://github.com/unconed/mathbox/blob/0.0.5/src/render/meshes/base.coffee#L120-L186... but they really read the same.

@ChristopherChudzicki
Copy link
Collaborator Author

ChristopherChudzicki commented Apr 11, 2022

On 0.0.5, setting lineX or lineY to true vs false has no effect. the lines are missing in both cases. THAT feels like a bug to me in 0.0.5

I'm pretty sure this does have an effect in 0.0.5, although the effect is not visible unless you're using a shaded surface. If not using shaded surface, the gridlines and surface are exact same color and you can't differentiate between the two. (I think the original intent was that the grid lines would be. adarker shade of the main surface color. I don't understand why the gridlines turned black, but I think I do understand why they were not behaving as intended in 0.0.5. Changing the x/y/z in this code to r/g/b fixes the lineX lineY issue in current master, and changing the corresponding lines in the old coffeescript code fix it in 0.0.5.

if (fill) {
const c = this.wireScratch;
c.setRGB(color.x, color.y, color.z);
this._convertLinearToGamma(
this._convertGammaToLinear(c).multiplyScalar(0.75)
);
this.wireColor.x = c.r;
this.wireColor.y = c.g;
this.wireColor.z = c.b;
}
}
. It would be nice to understand why the turned balack, but I'll settle for understanding why they weren't working as intended originally...).

As for the why the shaded surface itself is darker... I spent a while investiating that this weekend. I still do not fully understand. The shading is done by the two GLSL shader snippts surface.position.normal and mesh.fragment.shaded. . I'm a GLSL noob, but my understanding is that the "inputs" to these shader snippets are the "uniforms", and the current master shaders vs 0.0.5 appear to be getting the same inputs.

As best I can tell, there's an issue with gl_FrontFacing, though I really don't understand why it was working in ThreeJS r71 but not ThreeJS r137. Sigh. See #26 for more.

I'll open a PR for the lineX, lineY thing soon, and hopefully get back to the mathbox-react bug you opened. (Thanks for opening that!)

@sritchie
Copy link
Collaborator

Fixed by #27.

@sritchie sritchie mentioned this issue Jan 19, 2023
3 tasks
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

No branches or pull requests

2 participants