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

ColladaLoader: Re-added normal map parsing #22647

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

jbaicoianu
Copy link
Contributor

Related issue: #15391

Description

It seems that at some point, support for loading normal maps from Collada files was lost. This pull request restores that functionality to a working state, by parsing the material technique extras to handle the 'bump' option.

@jbaicoianu jbaicoianu changed the title Re-added normal map parsing to ColladaLoader ColladaLoader: Re-added normal map parsing Oct 6, 2021
@mrdoob mrdoob added this to the r134 milestone Oct 6, 2021
@mrdoob mrdoob merged commit 415a3b1 into mrdoob:dev Oct 6, 2021
@mrdoob
Copy link
Owner

mrdoob commented Oct 6, 2021

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Oct 6, 2021

I wonder how it got lost though...

@jbaicoianu
Copy link
Contributor Author

I wonder how it got lost though...

Yeah I was digging through repo histories last night trying to figure that out too, but couldn't find the "missing" code anywhere except in my own project's copy. It may have been that when we switched from ColladaLoader to ColladaLoader2 (and renamed ColladaLoader2 to ColladaLoader), I noticed that it was broken and ported what had previously worked over, but it wasn't actually working, and I fixed it in my local copy but forgot to contribute.

Guess it shows not a lot of people are using Collada anymore, glTF has definitely superseded it in wider use, but there's still a lot of old JanusVR content that uses it and I'm stubborn about keeping old content working 😄

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.

3 participants