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

[BREAKING] Remove StandardMaterial.diffuseTint and simply always apply it #6759

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Jun 27, 2024

  • before: When the diffuseTint is true, the diffuse color is used. But only when diffuse texture nor vertex colors were used. This is too confusing, and generates shader variants to save on a uniform and a single multiply.
  • now: tint is always on, and applied even when diffuse texture or vertex colors are used

A separate PR will apply this to additional tints of the material.

@mvaligursky mvaligursky merged commit bb4f3f7 into main Jun 27, 2024
8 checks passed
@mvaligursky mvaligursky deleted the mv-remove-diffuse-tint branch June 27, 2024 14:02
@Maksims
Copy link
Contributor

Maksims commented Jun 27, 2024

This change has a strong potential to break many materials for people, as they might have tweaked diffuse color, especially when it is set to weird greys from imported models often.

I've checked in Editor some of the projects with this small snippet:

editor.assets.list().filter((a) => {
    return a.get('type') === 'material'
        && a.get('data.diffuseTint') === false
        && a.get('data.diffuseMap') !== null
        && (a.get('data.diffuse.0') !== 1 || a.get('data.diffuse.1') !== 1 || a.get('data.diffuse.2') !== 1);
}).length

And I sometimes get 0, but some projects have dozens of materials that will be affected.

@mvaligursky
Copy link
Contributor Author

Hi @Maksims - yes, we'll be adjusting the materials in the Editor when the project loads with engine 2, and handle it. If the tint switch is false, we'll set the color to white.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants