Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Add the low-poly fox model #249

Merged
merged 4 commits into from
Jan 30, 2020
Merged

Add the low-poly fox model #249

merged 4 commits into from
Jan 30, 2020

Conversation

emackey
Copy link
Member

@emackey emackey commented Jan 29, 2020

Based on the one @scurest cleaned up in #150.

@emackey emackey mentioned this pull request Jan 29, 2020
@emackey
Copy link
Member Author

emackey commented Jan 29, 2020

I completely removed the old "Monster" model in #245, as it had serious issues beyond just conversion errors, such as a broken animation cycle, bad scaling, bad front direction, etc. This fox appears to be a much better sample on many levels.

@emackey emackey merged commit 3811e77 into master Jan 30, 2020
@emackey emackey deleted the fox-sample branch January 30, 2020 15:34
@prideout
Copy link

Maybe the README for this sample should mention the flat-shading aspect of the model, and that an implementation is incorrect if draws the fox with smooth shading?

@prideout
Copy link

Btw thanks for replacing the Monster model, this is much better :)

@emackey
Copy link
Member Author

emackey commented Jan 31, 2020

Yes, this model lacks normal vectors, which is unintentionally controversial as I don't think we previously had any models here that were both lit and lacking for normals.

The spec states that clients are expected to compute flat-shaded normals in this case. But this doesn't make sense for certain clients, as it adds not only the processing of the normals but also the need to split apart shared vertices. There are team members on both Cesium and STK who argue that the lack of normals would be better interpreted as being unlit, rather than requiring all the processing overhead of splitting shared vertices and calculating all the flat normals. But that's too large of a breaking change to retrofit into the official glTF 2.0 spec, so that issue is at an impasse for them.

In any case, the fox is here to demo multiple animation cycles, not lack of normals. Probably I should add the normals to the fox, and make a separate model to test for lack of normals.

@prideout
Copy link

Interesting! Thanks for explaining the controversy. For what it's worth, here on the Filament team we received a bug report (link) due to the lack-of-normals in this model, so I can imagine other renderers running into the same thing.

Anyway, I think your plan to split this into two tests makes sense and thanks again for adding this model -- it's a nice test!

@emackey
Copy link
Member Author

emackey commented Jan 31, 2020

Awesome! When you get a chance, let me know what your team thinks of #251. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants