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

Add renderer parameter to GLTFExporter #11964

Closed
wants to merge 1 commit into from

Conversation

fernandojsg
Copy link
Collaborator

@fernandojsg fernandojsg commented Aug 16, 2017

Fixes #11962

@mrdoob
Copy link
Owner

mrdoob commented Aug 16, 2017

Hmm... This renderer dependency doesn't feel right.

Considering that gl constants do not change, I think it would be better to create a map like in GLTFLoader.

https://github.com/mrdoob/three.js/blob/dev/examples/js/loaders/GLTFLoader.js#L771-L791

Same thing for WebGLUtils...

@fernandojsg
Copy link
Collaborator Author

fernandojsg commented Aug 16, 2017

I had it like that in the beginning but I changed it to reuse the WebGLUtils class. For me it also looks strange that we need to keep the list of constants everywhere we use it instead of having a WebGLUtils or so we we could do the match.
Maybe having the list of constants with it number directly instead of accessing the webgl context? Similar to https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/WebGLConstants.js

Also please note that if the following iterations of the exporter we'll need the renderer to access the textures directly, transform them and read the pixels back to store within the file.

@mrdoob
Copy link
Owner

mrdoob commented Aug 16, 2017

Maybe having the list of constants with it number directly instead of accessing the webgl context?

Sounds good to me 👌

Also please note that if the following iterations of the exporter we'll need the renderer to access the textures directly, transform them and read the pixels back to store within the file.

Lets try to keep the exporters renderer agnostic until is just not possible.

@fernandojsg
Copy link
Collaborator Author

Ok, I'll close it and create another PR with the proposed changes

@fernandojsg fernandojsg deleted the fix11962 branch August 16, 2017 22:22
@mrdoob mrdoob changed the title Fix #11692 add renderer parameter to GLTFExporter Add renderer parameter to GLTFExporter Aug 16, 2017
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

Successfully merging this pull request may close these issues.

2 participants