-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
Move WEBGL_CONSTANTS and THREE_TO_WEBGL constants out from GLTFExporter and GLTFLoader #11978
Conversation
The variables are glTF specific, correct? |
@takahirox these are not gltf specific, these are generic WEBGL constants (https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/Constants) that can be used in other places not just in the gltf loader. |
Oops, I didn't know that they're equal to |
Yes, the point was to avoid sending a |
I see I see, that makes sense! |
src/Three.js
Outdated
@@ -156,5 +156,8 @@ export { Curve } from './extras/core/Curve.js'; | |||
export { ShapeUtils } from './extras/ShapeUtils.js'; | |||
export { SceneUtils } from './extras/SceneUtils.js'; | |||
export { WebGLUtils } from './renderers/webgl/WebGLUtils.js'; | |||
export { WEBGL_CONSTANTS } from './renderers/webgl/WebGLConstants.js'; |
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.
I think I would keep this as:
export { WebGLConstants } from './renderers/webgl/WebGLConstants.js';
Also, I think I would add a fromGL()
and toGL()
methods that does the THREEToWebGL
and WebGLToTHREE` conversions. Just to keep everything in one file.
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.
@mrdoob Do you mean including and exporting fromGL
, toGL
and WebGLConstants
from WebGLConstants.js
?
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.
If that's the case, maybe rename the export to constants
so you could use it as:
THREE.WebGLConstants.constants.POINTS
THREE.WebGLConstants.fromGL(gl.POINTS)
THREE.WebGLConstants.toGL(THREE.FrontSide)
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.
I like this API style better.
var constants = new THREE.WebGLConstants();
constants.POINTS;
constants.fromGL(gl.POINTS);
constants.toGL(THREE.FrontSide);
But I'm not 100%. Maybe fromGL()
and toGL()
could go on WebGLUtils
instead.
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.
I don't really like the need to create a new instance just to query a constant. I would prefer to access it directly:
THREE.WebGLConstants.POINTS;
The same for the fromGL
and toGL
that I believe too that they should be part of WebGLUtils
THREE.WebGLUtils.fromGL(gl.POINTS)
THREE.WebGLUtils.toGL(THREE.FrontSide)
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.
@mrdoob I've updated WebGLUtils
and WebGLConstants
(still WIP) but to show a possible implementation of this last option
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.
Eventually convert
and toGL
will become the same function. In fact right now with the current modifications convert
doesn't need a gl
context anymore just the extensions
dependency.
c23d6f5
to
afe46fc
Compare
@mrdoob I've updated the Taking a look at |
Will this PR be merged soon? |
eb02143
to
7cfdced
Compare
Rebased! |
42771a7
to
7cfdced
Compare
I'm not sure why the |
7cfdced
to
2ff2690
Compare
|
@takahirox yep, I just rebased it and pushed it again :) |
ab3400c
to
a8ac02d
Compare
When will we merge this PR? |
minFilter: THREE_TO_WEBGL[ map.minFilter ], | ||
wrapS: THREE_TO_WEBGL[ map.wrapS ], | ||
wrapT: THREE_TO_WEBGL[ map.wrapT ] | ||
magFilter: THREE.WebGLUtils.toGL[ map.magFilter ], |
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.
toGL
is a function 😅
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.
fixed 👼
examples/js/loaders/GLTFLoader.js
Outdated
@@ -1381,7 +1264,7 @@ THREE.GLTFLoader = ( function () { | |||
return parser.getDependency( 'bufferView', accessor.bufferView ).then( function ( bufferView ) { | |||
|
|||
var itemSize = WEBGL_TYPE_SIZES[ accessor.type ]; | |||
var TypedArray = WEBGL_COMPONENT_TYPES[ accessor.componentType ]; | |||
var TypedArray = THREE.WEBGL_TO_THREE[ accessor.componentType ]; |
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.
Wouldn't it be better to use the same approach everywhere? THREE.WebGLUtils.fromGL()
...
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.
fixed
src/renderers/WebGLRenderer.js
Outdated
@@ -31,7 +31,7 @@ import { Vector3 } from '../math/Vector3.js'; | |||
import { WebGLClipping } from './webgl/WebGLClipping.js'; | |||
import { Frustum } from '../math/Frustum.js'; | |||
import { Vector4 } from '../math/Vector4.js'; | |||
import { WebGLUtils } from './webgl/WebGLUtils.js'; | |||
import { WebGLUtils } from './webgl/WebGLUtils'; |
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.
Lets keep WebGLUtils.js
.
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.
fixed
//------------------------------------------------------------------------------ | ||
var THREE_TO_WEBGL = { | ||
|
||
[ Constants.NearestFilter ]: WebGLConstants.NEAREST, |
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.
Was not aware of this [ variable ]
thing. Is it well supported?
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.
It's ES6 computed literals. I thought gulp would do the transform itself but it doesn't seems so :\
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.
The ugly but compatible version is to put the value of the constant as is, or try to use a gulp pluging to convert it to its literal value when creating the bundle. @mrdoob thoughts?
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.
My fault, this is es2015 and it's supported in all the browsers, but on firefox we need to add extra parenthesis: [ (value) ]
. I'll fix it
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.
actually, on the latest version of nightly it works as expected :)
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.
@mrdoob my fault, I was testing it incorrectly on firefox O:) it's working as expected in all the browsers, also tested on mobile (android & ios).
9f60f23
to
f9fb91b
Compare
f9fb91b
to
fe03d53
Compare
fe03d53
to
bf090a6
Compare
Closing this for now. |
Not so happy with the name
THREEToWebGL.js
andWebGLToTHREE
any suggestions? :)I believe most of the mappings from
WebGLUtils
(https://github.com/mrdoob/three.js/blob/dev/src/renderers/webgl/WebGLUtils.js#L13-L35 ...) could be replaced by this mappings constants as I find it more handy to use than need to create aWebGLUtils
instance with arenderer
andextensions
. Maybe in another PR we could just reuse the constants mapping on the WebGLUtils function and just leave the code related to extensions there?