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

Fog and Whites r72 #7111

Closed
titansoftime opened this Issue Sep 6, 2015 · 40 comments

Comments

Projects
None yet
5 participants
@titansoftime
Contributor

titansoftime commented Sep 6, 2015

It seems that whiter colors can pierce through the fog in 72. This was not an issue in 71.

http://jsfiddle.net/titansoftime/nu1217ch/

Changing to the master build provides expected results. This bug makes scenes with fog (most of mine) look kinda odd.

@arose

This comment has been minimized.

Contributor

arose commented Sep 6, 2015

Can't see the your example: XMLHttpRequest cannot load http://www.titansoftime.com/utils.php?task=getTexture&id=116. No 'Access-Control-Allow-Origin' header is present on the requested resource.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Sep 6, 2015

Can't see the your example

Fixed.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Sep 6, 2015

I noticed the reverse to be true as well. Dark colors pierce through whiter shadows.

http://jsfiddle.net/titansoftime/fw02moxy/

@mrdoob

This comment has been minimized.

Owner

mrdoob commented Sep 6, 2015

@WestLangley maybe this has something to do with the color space change?

@arose

This comment has been minimized.

Contributor

arose commented Sep 6, 2015

Changing the the depth directly in the shader seems to rescue it.

float depth = 130.0;  // gl_FragCoord.z / gl_FragCoord.w;

So, maybe it has something to do with the animation changing the coordinates? Do you see this effect also with static meshes?

@WestLangley

This comment has been minimized.

Collaborator

WestLangley commented Sep 6, 2015

@WestLangley maybe this has something to do with the color space change?

That was the first thing that came to my mind, too, but renderer.gammaInput is false in the demos.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Sep 6, 2015

So, maybe it has something to do with the animation changing the coordinates? Do you see this effect also with static meshes?

That doesn't seem to be it.

http://jsfiddle.net/titansoftime/nu1217ch/2/

@arose

This comment has been minimized.

Contributor

arose commented Sep 6, 2015

Seems ok with skinning switched off (which triggers all the animation staff, right?)

var mat = new THREE.MeshPhongMaterial({map:tex,skinning:false});
@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Sep 6, 2015

Seems ok with skinning switched off (which triggers all the animation staff, right?)

That's it. Yup looks fine with mat.skinning = false;. Well fine but non-animated heh.

@WestLangley

This comment has been minimized.

Collaborator

WestLangley commented Sep 7, 2015

What does skinning have to do with it?

@arose

This comment has been minimized.

Contributor

arose commented Sep 7, 2015

What does skinning have to do with it?

That is the question, for now it is just an observation that with skinning disabled there is no odd effect.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Sep 16, 2015

Anyone have any ideas on this?

This really makes my scenes look stupid, and again was not an issue till r72.

@mrdoob mrdoob added the Bug label Sep 16, 2015

@mrdoob

This comment has been minimized.

Owner

mrdoob commented Sep 17, 2015

It happens with both Fog and FogExp2. Also with MeshBasicMaterial, MeshLambertMaterial and MeshPhongMaterial.

I was going to say that maybe skinning is normalising the normals, but I don't think that's it.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 23, 2015

Update fiddle for 74dev: http://jsfiddle.net/d6mjj33a/

Old expected behavior (r71): http://jsfiddle.net/7hjo3u7k/

@WestLangley

This comment has been minimized.

Collaborator

WestLangley commented Nov 23, 2015

I'm not seeing anything poking through the fog.

But your AmbientLight color is 0xffffff. That is suspicious.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 23, 2015

But your AmbientLight color is 0xffffff. That is suspicious.

Why is that?

r74dev (You may need maximize this to see)
74

r71
71

I tested in this fiddle http://jsfiddle.net/xdc1c4xq/ changing AmbientLight to 0xaaaaaa. The issue remains.

@WestLangley

This comment has been minimized.

Collaborator

WestLangley commented Nov 23, 2015

The object of interest is 10 pixels high. I changed the camera field-of-view.

I also changed the fog constant and stopped the animation, as the amimations are different in the two fiddles. Also removed the directional light.

The fog appears to be affecting the brighter colors the most which looks suspeciously like a gamma-related issue. I am not sure how that can be, however, given the demo settings.

updated r.71: http://jsfiddle.net/7hjo3u7k/1/

updated r.74dev: http://jsfiddle.net/d6mjj33a/1/

BTW, I get this warning in the console:

THREE.WebGLRenderer: WEBGL_compressed_texture_pvrtc extension not supported.
@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 23, 2015

The object of interest is 10 pixels high. I changed the camera field-of-view.

In this example the object of interest may look insignificant, but in application where there are hundreds of objects in the fog, it looks quite silly. Changing the field of view appears to just hide the issue in this particular example. I don't believe this actually solves the problem, but instead sweeps it under the rug.

I also changed the fog constant and stopped the animation, as the animations are different in the two fiddles.

I am not sure which animation is playing is relevant. I will make sure they are playing the same animation however when I get home from work to prove this. The playing of an animation is important as the effect looks even worse with it.

The fog appears to be affecting the brighter colors the most which looks suspeciously like a gamma-related issue. I am not sure how that can be, however, given the demo settings.

The opposite effect is true as well. Through a lighter fog (0xdddddd ) for example, the issue is reversed where darker objects inappropriately "poke" through.

I would like it if one of you guys would recognize (as @arose) pointed out; that setting material.skinning to false "fixes" the issue, whereas simply not playing the animation does not. This leads me to believe (though I am probably wrong) that this is a material or some subset of a material issue.

@WestLangley

This comment has been minimized.

Collaborator

WestLangley commented Nov 23, 2015

Apparently you do not realize that I am trying to identify the cause of the problem for you. : - )

To do that, I needed to create an example that was actually visible. I also needed to eliminate parts of the code that appear to have nothing to do with the issue.

Yes, setting skinning: false "fixes" the issue.

three.js .r71 with some fog, skinning true, ambient light only, and no material.map: http://jsfiddle.net/7hjo3u7k/2/.

Dev branch with some fog, skining true, ambient light only, and no material.map: http://jsfiddle.net/d6mjj33a/2/.

EDIT: I am not sure which one is correct -- or if neither are.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 23, 2015

Apparently you do not realize that I am trying to identify the cause of the problem for you. : - )

I know =]

To do that, I needed to create an example that was actually visible. I also needed to eliminate parts of the code that appear to have nothing to do with the issue.

It is impossible to tell if either are correct due to the fact that the mesh in both cases is out of sight with the altered field of view. This is what I meant by swept under the rug. It look fine if the mesh is not visible heh =] Disabling fog in both of your updated examples will reveal that the mesh is outside of frustum.

EDIT. NEVERMIND... sigh. I am at work and access to my site via domain name is restricted..

Back to the issue... (updated to domain name to IP hehe, stupid firewall)

I believe your edits make better light of the issue (no pun intended). I believe the 71 example is correct though I cannot say this for certain. One would think this is the appropriate look considering there is no directional light or map. The 74 version looks like the fog is somehow being adjusted by a non-existent directional light. I'm seeing depth when I should not, correct?

All I can say is in a full scene that has a terrain of dimensions anywhere from 2000 x 2000 with several moving animated objects looks just bad in fog (from a distance, objects up close seem fine) using 72+ compared to 71 and prior.

@WestLangley

This comment has been minimized.

Collaborator

WestLangley commented Nov 23, 2015

The 74 version looks like the fog is somehow being adjusted by a non-existent directional light.

Not exactly. Fog is a function of depth. The depth appears to be (nearly or exactly) constant in the 71 version and highly variable in the 74dev version.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 23, 2015

Not exactly. Fog is a function of depth

Interesting. With this in mind, it would appear that the depth of the fog is not entirely taking into consideration the distance from the camera relative to an animated object (superficial observation). What really baffles me is why material.skinning affects this.

@mrdoob

This comment has been minimized.

Owner

mrdoob commented Nov 24, 2015

Just as in #7197, if you don't convert to BufferGeometry it seems to render just fine:

http://jsfiddle.net/d6mjj33a/6/

@mrdoob

This comment has been minimized.

Owner

mrdoob commented Nov 24, 2015

Just as in #7197, my bet is that SkinnedMesh.normalizeSkinWeights() expects skinWeights to be normalized when using BufferGeometry.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 24, 2015

You are correct, though your fiddle is broken (assigns geo2 but uses geo).

The fix (with proper render): http://jsfiddle.net/90g09p4r/

Using BufferGeometry definitely is the culprit for both issues. Is there a way to normalize the skin weights already in three.js (not seeing a specific method on quick glance) ? If not I'll play around with it this weekend when I have some free time ( unless of course you far more brilliant people beat me to it =] ).

So as you pointed out there is normalizeSkinWeights. Little confused here.

my bet is that SkinnedMesh.normalizeSkinWeights() expects skinWeights to be normalized when using BufferGeometry.

If SkinnedMesh.normalizeSkinWeights() normalizes the skinWeights, why would it expect already normalize weights? Is the intent to normalize the normals (which I admit is a foreign concept to me)?

@WestLangley

This comment has been minimized.

Collaborator

WestLangley commented Nov 24, 2015

Hack to pre-normalize weights and use BufferGeometry. It appears to work.

r.74dev: http://jsfiddle.net/d6mjj33a/7/

Compare with r.74dev without pre-normalizing: http://jsfiddle.net/d6mjj33a/2/

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 25, 2015

It appears to work.

Indeed it does. Everything looks great in app after applying the hack. In fact it fixed another issue I was having in addition to #7197 that I have not posted yet for I wasn't yet sure if it was of my doing =]

As for the next step I suppose normalizeSkinWeights could be moved to Geometry as BufferGeometry currently does not need it due to the fact that the exporter does not export BufferGeometry with skinWeights (yet or as far as I know). This way we could call normalizeSkinWeights in BufferGeometry.fromGeometry. Eh?

@WestLangley

This comment has been minimized.

Collaborator

WestLangley commented Nov 25, 2015

The issue of normalizing skeletal animation weights has been discussed in #6178.

What should our policy be: asssume the weights are normalized or auto-normalize?

/ping @bhouston Opinion?

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 25, 2015

asssume the weights are normalized

Can this assumption really be made? Perhaps the current Blender exporter supplies normalized weights, the old one (r68) does not. I would prefer not to re-export hundreds of meshes if avoidable. The current exporter has so many problems with skeletal animations (particularly with multiple animations) that this wouldn't really be a viable option anyway. So I beg you to consider auto-normalization for the sake of backwards compatibility.

All that being said; the hack works well enough, so do what you must =]

@bhouston

This comment has been minimized.

Contributor

bhouston commented Nov 25, 2015

I believe we can just normalize the resulting normal after the skinning code in the glsl code? Always normaling the normal once after all of our modifications (displacement, normal mapping, bump mapping, skinning) is likely a good idea anyhow. Shouldn't cost much at all either.

@bhouston

This comment has been minimized.

Contributor

bhouston commented Nov 25, 2015

I think as well have an optional function call to "normalizeSkinWeights()" is a useful function as well and won't hurt. But it would be optional because it will slow down loading unnecessarily when people know they already have good normals.

@bhouston

This comment has been minimized.

Contributor

bhouston commented Nov 25, 2015

What likely happened is that we removed an normalization of the normal somewhere that used to be there. We should just add it back.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 25, 2015

What likely happened is that we removed an normalization of the normal somewhere that used to be there. We should just add it back.

This all came from using BufferGeometry with skeletal animations, which apparently requires assumes the skin weights to be normalized. Prior to r72, using BufferGeometry this way was not the most straight forward task which kept this bug from coming to light. Geometry does not seem to have this requirement (perhaps as you said there is some normalization of the weights somewhere along the path to render?).

I think as well have an optional function call to normalizeSkinWeights()

This would be great if the method belonged to Geometry, this would remove the need for WestLangley's hack.

@WestLangley

This comment has been minimized.

Collaborator

WestLangley commented Nov 25, 2015

I believe we can just normalize the resulting normal after the skinning code in the glsl code?

@bhouston If the weights are not normalized, this affects not only the normals but the vertex positions too. Correct?

Is it ever appropriate to have a model with non-normalized weights?

This all came from using BufferGeometry with skeletal animations, which apparently requires the skin weights to be normalized.

@titansoftime I think is would be more correct to say that BufferGeometry assumes the weights are normalized.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 25, 2015

I think is would be more correct to say that BufferGeometry assumes the weights are normalized.

Yes, agreed.

@bhouston

This comment has been minimized.

Contributor

bhouston commented Nov 25, 2015

@bhouston If the weights are not normalized, this affects not only the normals but the vertex positions too. Correct?

Are only the normals affected or the positions as well? The two jsfiddles you posted seemed to have the same positions, just the normals changed. Or maybe the positions changed too and it was very subtle.

Is it ever appropriate to have a model with non-normalized weights?

It happens. Would be cool to be robust in that situation if it didn't incur a runtime penalty and was easy to do. But I am not really able to engage on this topic, so what ever you guys decide.

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 25, 2015

The two jsfiddles you posted seemed to have the same positions, just the normals changed. Or maybe the positions changed too and it was very subtle.

Taken from #7197:

Looks much better if weights are normalized prior to converting to BufferGeometry.
r.74dev: http://jsfiddle.net/eo0hpjk5/2/
r.71: http://jsfiddle.net/fnanr01f/
However, now notice that there are slight differences in the animations between versions.

I've been staring at this animation for over a year or so and can barely tell the difference, but there seems to be a subtle one.

@WestLangley

This comment has been minimized.

Collaborator

WestLangley commented Nov 25, 2015

OK. Here is what I propose we do: continue to normalize the weights in the SkinnedMesh constructor, and add support for normalizing weights when the geometry is BufferGeometry.

@mrdoob Does that make sense to you?

However, now notice that there are slight differences in the animations between versions.

Unless I am mistaken, the new animation system produces different vertex positions than the old one (#7197 -- look at the sail).

@mrdoob

This comment has been minimized.

Owner

mrdoob commented Nov 25, 2015

@mrdoob Does that make sense to you?

Sounds good!

@titansoftime

This comment has been minimized.

Contributor

titansoftime commented Nov 26, 2015

Fixed in #7679

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