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

Use the vertex colors by default in gltf. #41007

Merged
merged 1 commit into from
Sep 28, 2020

Conversation

fire
Copy link
Member

@fire fire commented Aug 3, 2020

Fixes #21087

@fire
Copy link
Member Author

fire commented Aug 3, 2020

image

@fire fire force-pushed the gltf-vertex-colors branch from a58765f to 7bac401 Compare August 3, 2020 18:49
@aaronfranke aaronfranke added this to the 4.0 milestone Aug 5, 2020
@aaronfranke aaronfranke added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:import enhancement and removed bug labels Aug 5, 2020
@akien-mga
Copy link
Member

akien-mga commented Sep 3, 2020

17:01 <Akien> reduz: ok with #41007 ?
17:11 <reduz> that makes the shader more complex if you are not using the vertex colors
17:11 <reduz> so no
17:12 <reduz> also I have seen many times models that have vertex colors but they are not used, so when you load them the model looks wrong
17:13 <reduz> so in general, its better to enable vertex color import only if you are meaning to use them
17:13 <Akien> Can this be set as default import preset for users that want it?
17:13 <Akien> I mean from the import dock
17:13 <reduz> Akien: you can already set it as a default import preset, what we never did is a way to edit the default import pressets in the project settings
17:14 <reduz> we should probably do that
17:14 <reduz> so you can only override them, not change them
17:14 <Akien> Yeah that would be nice also for setting pixel art presets, etc.

Closing as per the above, it's best to leave it up to users to set a default import preset for glTF that uses vertex colors if they know they'll benefit from it systematically.

@akien-mga akien-mga closed this Sep 3, 2020
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Sep 3, 2020
@fire
Copy link
Member Author

fire commented Sep 3, 2020

The GLTF spec says vertex colors need to be mixed with materials, so I don't know how to solve this.

@capnm
Copy link
Contributor

capnm commented Sep 3, 2020

17:12 also I have seen many times models that have vertex colors
but they are not used, so when you load them the model looks wrong

The model is then exported incorrectly, it shouldn't contain any vertex colors ...
you can then fix that in the material-settings manually.

@fire
Copy link
Member Author

fire commented Sep 3, 2020

iFire> reduz: So for clarity is the decision still to not to have vertex colors default to mix with the material for glTF2? It's ok to say no, but being in decision limbo is worse.
10:35 AM
Juan Linietsky iFire: to be honest, gltf2 probably is ok
10:35 AM for collada and fbx i am less sure
10:36 AM
that's a good reason
10:36 AM
Juan Linietsky the problem is that we will need to detect this and turn it on in the material

@akien-mga
Copy link
Member

Well I thought that there was an import option for this, but obviously I must have been mistaken. Not sure what @reduz was referring to then.
Screenshot_20200903_195251

Let's reopen and assess further.

@akien-mga akien-mga reopened this Sep 3, 2020
@akien-mga akien-mga removed the archived label Sep 3, 2020
@fire
Copy link
Member Author

fire commented Sep 28, 2020

reduz> Juan Linietsky iFire: probably but the commits are kind of a mess, needs to be rebased

Will rebase when I have a free moment.

@fire fire force-pushed the gltf-vertex-colors branch from 7bac401 to 43424e1 Compare September 28, 2020 17:38
@akien-mga akien-mga merged commit b7066ef into godotengine:master Sep 28, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 28, 2020
@fire fire deleted the gltf-vertex-colors branch September 28, 2020 18:41
@capnm
Copy link
Contributor

capnm commented Sep 28, 2020

3.2 PR: #41149

(Sorry for the spam here, github keeps referencing old rebased commits and and doesn't have a [Garbage Collection] button)

@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

glTF: Use the vertex colors when available
4 participants