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

Bump Three.js to r84 #2456

Merged
merged 4 commits into from
Mar 6, 2017
Merged

Bump Three.js to r84 #2456

merged 4 commits into from
Mar 6, 2017

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Mar 5, 2017

Description:

This PR bumps Three.js to r84.

Because of THREE.GLTFLoader specification in r84 or newer,
.gltf and .glb files need to be loaded as arraybuffer.

I let a-asset-item automatically set arraubuffer response-type attribute for them.

I think this automation is necessary because of

  • Usability
  • A-Frame compatibility
  • Not confusing people who know .gltf is json based text data

FYI, THREE.GLTFLoader in r84

  • (partially) supports 1.1
  • can't load 2.0 (in r85, it (partially) supports 2.0)

Changes proposed:

  • Update Three.js to r84
  • a-asset-item detects glTF files from their extensions and automatically sets arraybuffer response-type attribute.

because of THREE.GLTFLoader specification.
*/
if (!responseType) {
var extension = '';
Copy link
Member

@ngokevin ngokevin Mar 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make this code a private exported function inferResponseType (src) and test on its own.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment.
What does 'test on its own' mean in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can test the function by calling it directly rather than creating a-asset-item

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very useful. I will update.

@takahirox
Copy link
Collaborator Author

Updated.

@ngokevin ngokevin merged commit 9d0e015 into aframevr:master Mar 6, 2017
@takahirox takahirox deleted the threeR84 branch March 6, 2017 04:45
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