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

GLTFLoader: Add basic example. #13055

Merged
merged 3 commits into from
Jan 8, 2018

Conversation

donmccurdy
Copy link
Collaborator

Fixes #13041.

@donmccurdy donmccurdy force-pushed the feat-gltfloader-helmet-example branch 2 times, most recently from debc1cd to 0a04437 Compare January 7, 2018 03:08
@donmccurdy donmccurdy force-pushed the feat-gltfloader-helmet-example branch from 0a04437 to 4e9fac6 Compare January 7, 2018 03:14
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jan 7, 2018

@donmccurdy donmccurdy force-pushed the feat-gltfloader-helmet-example branch from d40f480 to 193dacf Compare January 7, 2018 03:25
@takahirox
Copy link
Collaborator

takahirox commented Jan 7, 2018

Personally I want the option to turn on/off envMap in scene.background because new users would be confused by what the model reflects.


}
scene.add.apply( scene, gltf.scene.children );
Copy link
Collaborator

@takahirox takahirox Jan 7, 2018

Choose a reason for hiding this comment

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

Not strong preference but I prefer

scene.add( gltf.scene );

because I'm really not sure how well function.apply style is known to JS (especially new) users.

If we don't wanna push gltf.scene to scene, the code would be like this?

or

while( gltf.scene.children.length > 0 ) {

    scene.add( gltf.scene.children[ 0 ] );

}

(a bit complex?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure ✅

@soljurami
Copy link

What is thst?

@donmccurdy
Copy link
Collaborator Author

Personally I want the option to turn on/off envMap in scene.background because new users would be confused by what the model reflects.

I don't think it's worth adding a UI toggle to change the background, since that's not the point of the example, but what about this?

screen shot 2018-01-07 at 12 13 12 pm

@takahirox
Copy link
Collaborator

Looks good

@WestLangley
Copy link
Collaborator

@soljurami The description is at the top of the webpage.

@donmccurdy I would remove the background and add black or white fog if you want to keep the floor/shadow -- or remove the floor and set a black background. IMHO.

@takahirox
Copy link
Collaborator

@WestLangley Why you wanna remove the background? For the simplicity?

@looeee
Copy link
Collaborator

looeee commented Jan 8, 2018

I think the background looks good here, and adds only a couple of lines of code. Even in these simple examples it's OK to demonstrate a couple of other basic techniques, such as adding floors, shadows, environment maps.

@WestLangley
Copy link
Collaborator

Why you wanna remove the background?

I think the example floor and background in combination are ugly. My suggestions were based on aesthetics only.

The first screenshot above looks very nice to me. Very professional. I'd go with that.

@takahirox
Copy link
Collaborator

takahirox commented Jan 8, 2018

I can understand. TBH I had the same feeling, the first one looked aesthetically better. (But the second one looks better as example.)

I still want background because of the reason I mentioned.

Removing floor? Do you folks think shadow is important in this example?

image

No floor.

image

No floor and grid.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 8, 2018

If would pick the last option so no floor and grid.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jan 8, 2018

How about the floor+grid+sky of the first screenshot, with the skybox envMap replacing the park scene used before? I think if the reflections were just sky, rather than distinct trees, there's less potential for confusion. I do prefer the simple grid scene over any of the cubemaps, aesthetically..

screen shot 2018-01-08 at 12 52 48 pm

@donmccurdy donmccurdy force-pushed the feat-gltfloader-helmet-example branch from 55c0359 to 1780443 Compare January 8, 2018 20:53
@WestLangley
Copy link
Collaborator

Personally, I would not try to anticipate what new users would be confused by. I'd make the demo look as professional as possible.

I understand that what looks good aesthetically is a personal preference. I do not like the sky background at all. The first screenshot looks fine to me -- with either black or white fog. Just my opinion, though. :)

@mrdoob
Copy link
Owner

mrdoob commented Jan 8, 2018

Don't you worry about it guys! I'll make sure it looks professional 😎

@mrdoob mrdoob merged commit c9c68e7 into mrdoob:dev Jan 8, 2018
@mrdoob
Copy link
Owner

mrdoob commented Jan 8, 2018

Thanks!

@takahirox
Copy link
Collaborator

takahirox commented Jan 8, 2018

I just realized I've misunderstood a bit.
I wanted background for showing that glTF 2.0 PBR material has environment map feature but I'm aware of that glTF 2.0 PBR material doesn't mention anything about environment map.

So the example looks fine.

Update: environment map would be in extension but no in core KhronosGroup/glTF#946

@mrdoob mrdoob added this to the r90 milestone Jan 17, 2018
@mrdoob
Copy link
Owner

mrdoob commented Feb 13, 2018

Alright, after 25b71ec I think it looks a bit more "professional" 😁

screen shot 2018-02-13 at 3 27 13 pm

@donmccurdy donmccurdy deleted the feat-gltfloader-helmet-example branch March 7, 2018 15:19
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.

7 participants