Skip to content

Conversation

@looeee
Copy link
Collaborator

@looeee looeee commented Feb 15, 2018

Although now it should really be called webgl_materials_car since there's only one car model 😅

New version
Old version

@WestLangley
Copy link
Collaborator

The model seems to be missing parts.

Maybe do this as a workaround:

carModel.traverse( function( node ) { if ( node instanceof THREE.Mesh ) { node.material.side = THREE.DoubleSide } } );

Maybe find a better model?

@looeee
Copy link
Collaborator Author

looeee commented Feb 15, 2018

I've done quite a bit of work on the model so far, it still has a couple of missing bits \ tiny gaps but I'll fix them in a separate PR since this model is used across a couple of examples. It can mostly be fixed by adding a bottom plate to the car and adding an inside surface to the body.

BTW I did previously submit a different model (Dodge viper) but was told I should use a model already in the repo...

@takahirox
Copy link
Collaborator

Just curious to know what the main purpose of switching to glTF model is.

@looeee
Copy link
Collaborator Author

looeee commented Feb 16, 2018

We're working on deprecating the old BinaryLoader. The examples using these car models are the only remaining places it's used.

@takahirox
Copy link
Collaborator

Gotcha

@takahirox
Copy link
Collaborator

takahirox commented Feb 16, 2018

BTW do we still need materials_cars example?
I'm really not sure what the purpose of materials_cars example is.
To show Three.js can switch material at run time?

@looeee
Copy link
Collaborator Author

looeee commented Feb 17, 2018

I would be ok with removing it.

To be honest, there are quite a few examples like this, that either demonstrate something outdated, or don't really demonstrate any feature but just look good, or are duplicates of other examples.

While it's great to have this huge body of examples, having too many is actually counterproductive since they don't get updated and end up demonstrating out of date techniques (for example, many examples still use Geometry rather than BufferGeometry).

@takahirox
Copy link
Collaborator

Yeah, I think we can remove materials_cars example.

Any thoughts? @mrdoob

@looeee
Copy link
Collaborator Author

looeee commented Feb 27, 2018

There are now 3 PRs that use this updated car model: this one and #13084, #13314

Two questions have been raised:

1. As @WestLangley pointed out above there are some issues with this model - in particular the interior is missing a couple of pieces. I can either

  • continue to work on this model to improve it
  • replace it with on of the other models from webgl_materials_cars - probably the Bugatti Veyron since it looks like the other models are also missing pieces
  • find a new model, such as the Dodge Viper that I previously submitted here. I would prefer this one as its less work for me 😅

2. Is this example (materials_cars) worth keeping? Or should we remove it?

@mrdoob can you provide feedback here before I proceed in this?

@looeee
Copy link
Collaborator Author

looeee commented Mar 14, 2018

/ping @mrdoob for feedback.

@mrdoob
Copy link
Owner

mrdoob commented Mar 14, 2018

Sorry for the delay. This PR is looking good! I'll polish it in the next cycle 👌

@mrdoob mrdoob added this to the r92 milestone Mar 14, 2018
@looeee
Copy link
Collaborator Author

looeee commented Mar 14, 2018

Sorry for the delay

There no rush, just didn't want this to gather too much dust. 🙂

@Mugen87 Mugen87 mentioned this pull request Mar 21, 2018
3 tasks
@mrdoob mrdoob modified the milestones: r92, r93 Apr 18, 2018
@mrdoob mrdoob modified the milestones: r93, r94 May 22, 2018
@mrdoob mrdoob modified the milestones: r94, r95 Jun 27, 2018
@mrdoob mrdoob modified the milestones: r95, r96 Jul 31, 2018
@mrdoob mrdoob modified the milestones: r96, r97 Aug 29, 2018
@looeee
Copy link
Collaborator Author

looeee commented Sep 4, 2018

Closed in favour of #14843

@looeee looeee closed this Sep 4, 2018
@mrdoob mrdoob removed this from the r97 milestone Sep 4, 2018
@looeee looeee deleted the materials_car branch June 5, 2019 14:46
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.

4 participants