mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.69k stars 35.37k forks source link

Illogical Conditional in WebGLRenderer #5675

Closed titansoftime closed 9 years ago

titansoftime commented 9 years ago

Not a huge deal but this conditional in "setTextureParameters" seems to be wrong:

if ( texture.minFilter !== THREE.NearestFilter && texture.minFilter !== THREE.LinearFilter ) {

    console.warn( 'THREE.WebGLRenderer: Texture is not power of two. Texture.minFilter is set to THREE.LinearFilter or THREE.NearestFilter. ( ' + texture.sourceFile + ' )' );

}

Correct me if I'm wrong but won't this argument always be true? How can texture.minFilter === both THREE.NearestFilter and THREE.LinearFilter simultaneously?

mrdoob commented 9 years ago

/ping @BenediktS

titansoftime commented 9 years ago

Bah nevermind, I'm an idiot.

Datamance commented 9 years ago

@mrdoob Can you take this console warning out?

What kind of developer behavior is it supposed to encourage? If there's no good reason for this, it should be stripped from your distribution build. Calls to the console dramatically slow application runtime, especially if I'm making a bunch of textures in a loop.

theo-armour commented 9 years ago

I think I agree with @Datamance here.

I often build placards of text using textures built with a canvas. These textures are never square so they generate hundreds of console calls. It's nice to have this warning when developing but not so nice in production. It would be nice not to have them or have a simple way of turning them off in production scripts. . See http://tzigzagz.github.io/tzzz-grid-view/r1/tzzz-grid-view-r1.html

Datamance commented 9 years ago

Yeah, I'm literally patching this ATM:

THREE.LinearFilter = THREE.NearestFilter = texture.minFilter;

Though the visual artifacts from this are too subtle for me to notice (if there are any at all), it's still pretty gross to have in production code.

Actually, the same goes for console errors/warnings like this:

"THREE.Projector has been moved to /examples/js/renderers/Projector.js."

Beyond the point that you shouldn't be exposing library internals to client code... what are we supposed to do about this file change? I assume most people, like me, are using your bower/git distribution. Client code shouldn't have to specify things like, "from where am I supposed to retrieve this class of code?"

Please change this! Many thanks :)

mrdoob commented 9 years ago

@mrdoob Can you take this console warning out?

Pull Request for fixing this are more than welcome! :)

"THREE.Projector has been moved to /examples/js/renderers/Projector.js."

If you're seeing this message it mesans that you're creating a Projector which you shouldn't. Probably not the best message...

BenediktS commented 9 years ago

@theo-armour: But if you know that your texture are not power of two why don't you set the Texture.minFilter to THREE.LinearFilter and the warnings are all gone? I think a library should never change a quality setting without noticing the programmer about it.

If the condition rouse there is definitly something wrong with the texture or the minFilter.

@Datamance : It should encourage the developer to set the minFilter right or to use power of two textures. It should warn him that you could not use the other filters with a none power of two texture. If you use it you will not get the expected output.

And for speed purpose on many devices it should encourage the developer to use power of two textures ;)

mrdoob commented 9 years ago

Maybe we could change the warning to this?

console.warn( 'THREE.WebGLRenderer: Texture is not power of two. Texture.minFilter should be set to THREE.NearestFilter or THREE.LinearFilter. ( ' + texture.sourceFile + ' )' );
theo-armour commented 9 years ago

@BenediktS Thank you for the tip. This should solve the problem

Unfortunately, in the case I am working on it did not seem to work - because the texture is being generated on a canvas and not loaded from a file.

Since this not a bug, I will keep quiet here and continue on Stack Overflow...

BenediktS commented 9 years ago

@mrdoob: yes. I think your new message is better.

mrdoob commented 9 years ago

@BenediktS Thanks!