Better face normals for mesh3d

[etpinard]

fixes #1368

via @dfcreative 's gl-vis/gl-mesh3d#8 which now appears to behave well on iOS, Windows, Ubuntu (including on CircleCI).

Note the baselines diffs though. The new algorithm clearly changes the look of the 3D meshes. Is that for better or for worse? cc @monfera @rreusser @alexcjohnson @jackparmer

[alexcjohnson]

I wouldn't say it's better or worse, but sufficiently different to raise an alarm - it looks like the light source is now coming from exactly the opposite direction? Have we reversed a sign somewhere?

[rreusser]

It looks like the terminators are exactly the opposite in the snowden example so that yeah, seconding the flipped sign hypothesis.

[dy]

What is the chance that test images had wrong sign all the time due to rendering in Ubuntu?
I inverted sign in gl-vis/gl-mesh3d@c0fb6bc, but that breaks own gl-mesh3d tests @etpinard

[monfera]

I have a vague recollection of counterintuitive light direction, although I was using OS X, and it was one specific thing (Snowden). I'm wondering if the polygon (triangle) winding order might be anything to do with it, i.e. maybe with the Snowden the triangles are clockwise when seen from the 'outside', and counterclockwise for the others (or the other way around). Just a random thought in a semi-slumber...

[alexcjohnson]

If we can clearly demonstrate that the previous behavior is a bug then I suppose there's no problem cementing the new behavior. Presumably the default light location was chosen, in the context of the buggy interpretation, to be some standard-ish above-and-to-the-side illumination for the default camera angle? Should we then alter the default lighting at the same time as we fix the bug? Then the practical results would be limited to those folks who've explicitly set a light source - admittedly exactly the folks who are most likely to care, but a bug needs to be fixed...

If there's any chance the winding order affects any of this, we need to sort that out and fix it first.

[rreusser]

For debuggability and my own sake, here's a cube with definitely-outward facing normals (at least to the extent that it follows the right hand rule per-face): http://codepen.io/rsreusser/pen/PpPJWW?editors=0010

I'm actually kind of confused what lightposition does. It's clearly in screen space or something but such that it's not totally clear to me what space the light is oriented in.

Edit: I must be doing something wrong because lightposition seems to have no effect. I'm going to move on, but maybe this codepen is useful for debugging.

Double edit: It does have an effect. x = y = 100000 puts highlights in the upper right corner, which makes some sense. I still just don't understand what space those coordinates are in.

[etpinard]

The new baselines are here db6532c

Looking a lot better:

Left: master Right: with db6532c

[etpinard]

@dfcreative one possible red flag: the cube baseline now looks a bit darker

Is that the expected bahavior? It is just an Ubuntu / nw.js artefact?

[etpinard]

@dfcreative any news on my previous ⬆️ #1423 (comment) ?

I would ❤️ to merge this PR in the next few days.

[dy]

@etpinard I'll try to look at it tonight, but that seems to be not the only issue. It seems to be snowden' model issue.

[dy]

Turns out that default cube setup has broken lighting:

That is also true for the API demo:

The @monfera's guess is right, the triangles are in wrong order of vertices. Is there any theoretical background for the order or vertices? Maybe we should sort them before use? Out of ideas for now.

[rreusser]

The rule for triangle normals is just the right hand rule. I guess to fix it you'd have to build adjacency data, decide which direction is outward, and then flip normals to match the correctly oriented neighbors. Just a guess. There are plenty of edge cases (that's a pun) where trying to fix it automatically would probably do more harm than good though. I think it's totally fair and reasonable to state the required orientation and require correct input.

[alexcjohnson]

decide which direction is outward

so we can't do a klein bottle? why can't both directions be "outward"?

[rreusser]

@alexcjohnson Would the solution be to abs(*) instead of max(0, *) the diffuse lighting? The downside is that lights would illuminate from the source direction and also the anti-source direction.

[alexcjohnson]

lights would illuminate from the source direction and also the anti-source direction.

That doesn't sound good... the light needs to act like a real light source and come from one side, so it does its job of helping you to see the shape of the data.

Can't we just make the active surface normal be the one you see? Which I guess means the one that has a positive dot product with the vector from somewhere on the face to the camera?

[monfera]

I'm elbow deep in some other task now, but wanted to agree that as @rreusser said, working out face direction from global mesh or adjacency data would be prone to all kinds of issues, and as @alexcjohnson said, it's possible to do the lighting such that it's invariant of the (e.g.) right winded interpretation of the face normal, indeed as needed for rendering a semi-opaque Klein bottle.

I'm not certain how conducive the current lighting algo is to this (pls. correct me if I'm wrong but it's the diffuse lighting only i.e. not the specular), and I also found e.g. during the brain mesh work that semitransparent rendering didn't quite work out. So due to this we can't render a convincing semi-opaque Klein bottle.

Most likely unrelated, but there's also the possibility of face culling - I suppose we don't cull back faces as otherwise the triangle would be dropped. To render semi-opaque Klein bottles there cannot be face culling.

[etpinard]

Hmm. Sounds like this issue is from being resolved.

@jackparmer ok to drop this from the 1.25.0 release?

[etpinard]

Good point @dfcreative

Any opposition to merging this?

EDIT: by this I mean first merging gl-vis/gl-mesh3d#8

[monfera]

Yes I think it's a big improvement!

[etpinard]

New baselines with the latest gl-mesh3d commit gl-vis/gl-mesh3d@63dba35 are in 26751ec which are essentially the same as from the time of this gl-vis/gl-mesh3d#8 (comment)

I'm looking for some much needed 👍s before going ahead and merging this thing.

[alexcjohnson]

😿 the intermittent CI failure I saw yesterday is here again today:

TypeError: unsupported file type: undefined (file:
/var/www/streambed/image_server/plotly.js/build/test_images/gl3d_z-range.svg)

[alexcjohnson]

Snowden is still lit from the opposite side as before this change. The others too, probably, but the rest are hard to say anything about except "they changed."

So I'll ask again: which is correct and which is a bug? I understand there may be other advantages to this change, but we cannot just flip the lighting on all of our 3d surfaces without clearly determining that the previous behavior was a bug.

[rreusser]

FYI: I just learned about this function which might make the klein bottle work and is presumably safer than gl_FrontFacing because it's just a straight math function vs. something fancy: http://docs.gl/el3/faceforward

[monfera]

@rreusser neat! looks like a WebGL 2 thing, or maybe already prevalent?

[rreusser]

@monfera it says OES 1.0, which should be webgl 1 unless I'm mistaken. Which isn't unlikely.

[monfera]

@rreusser cool, I just saw the GLSL ES 3.1 label on top, but indeed, 1.0 is listed below

[rreusser]

@alexcjohnson I'll see if I can phrase it as a challenge. Can someone demonstrate clear behavior/intent/consistency with the lighting position in this codepen? Against plotly-latest? Against this PR? http://codepen.io/rsreusser/pen/PpPJWW?editors=0010

It's a cube with normals just averaged to point outward so that it gets shaded like a sphere. (I thought it through carefully and used the righthand rule to orient the faces consistently outward, so I really hope that part is correct.) Reverse engineering, here's what I know:

• (x, y, z) seems to be in screen space.
  • (x,y,z) = (-100000, 0, 0) places the light to the left, which makes sense
  • (x,y,z) = (100000, 0, 0) places the light to the right, which makes sense
  • (x,y,z) = (0, 100000, 0) lights from above
  • (x,y,z) = (0, -100000, 0) lights from below
  • I can't figure out what z does to the lighting position
  • I don't know quite why the numbers have to be so big. Are the positions in screen pixels?

Of course the real approach should be to debug the code/intent itself, but this is my understanding from treating it as a black box, which is what users would do.

[rreusser]

After digging into the behavior quite a bit with @alexcjohnson, here's what we conclude:

The current behavior mostly make sense. The really weird stuff is all in the light positioning.

• the magnitudes have to be really giant. Are they pixels or something?
• z exhibits really weird behavior and thresholds

Apart from how it does work, here's how we think it should work:

The light should be positioned effectively in clip coordinates with the addition of an aspect ratio. Conceptual test cases:

• a light position of [1, 0, 0] should be just on the right side of the screen (at the depth of the center, that is. the projection matrix makes this statement otherwise meaningless). It involves an (inverse?) view matrix somewhere in there, I think.
• a light position of [10, 0, 0] would be roughly speaking 10 half-screens worth off to the right (again, at the depth of/relative to the center point with respect to the alignment of the eye)
• a light position of [0, 1, 0] should be at the top of the screen. If the plot is not square, then one or the other of these will need to take into account an aspect ratio.
• a light position of [1, 1, 0] should be at 45 degrees NE regardless of the plot's aspect ratio. This constraint probably determines where the aspect ratio goes
• a light position of [0, 0, 1] should be more or less at the eye position, unless I'm wrong about scales. At the very least in the direction of the eye. Out of the screen instead of into because of the righthand rule.
• a light position of [0, 0, 10] should be way behind the camera
• If we can, it'd be great of length(light) > 1 implied the light tended not to fall inside the screen (I'm not 100% convinced this can be satisfied subject to the other constraints)

Note: scene aspect ratio and axis ranges seem to scale things entirely independently of the view, so I think they can be ignored.

Open to feedback, but I think these constraints are enough to minimally fix the parts of the current behavior that are surprising without changing anything substantial. In talking with @alexcjohnson one conclusion is that existing behavior like the weird 1.00002 -> 1.00003 z threshold is best just fixed so that it's probably best not to preserve strict backward compatibility.

[etpinard]

Dropping from the 1.25.0 milestone.

To minimize the impact on our users, we should address @rreusser 's lightpositon #1423 (comment) proposal in the same release as @dfcreative 's hardware-dependent fixes done in this PR.

[etpinard]

Closing this thing for now.

I'll copy #1423 (comment) in #1368 . Future development should refer to #1368