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

Fix instance color forwarding for PBR Instances #12333

Merged
merged 1 commit into from
Apr 4, 2022
Merged

Fix instance color forwarding for PBR Instances #12333

merged 1 commit into from
Apr 4, 2022

Conversation

CedricGuillemet
Copy link
Contributor

Followup https://forum.babylonjs.com/t/vertex-error-and-no-colors-on-thin-instances-after-upgrade/28492/13
Instance colors were not properly forwarded for PBR materials

@azure-pipelines
Copy link

We have noticed you haven't changes the "what's new.md" file. If your update is important (a bug fix, a new feature), please make sure to update the what's new file in the base directory and commit the changes.

@azure-pipelines
Copy link

Snapshot stored with reference name:
refs/pull/12333/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12333/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12333/merge/index.html#WGZLGJ#4600

To test the snapshot in the playground itself use (for example):

https://playground.babylonjs.com/?snapshot=refs/pull/12333/merge#BCU1XR#0

@sebavan sebavan merged commit 70c4f53 into BabylonJS:master Apr 4, 2022
@@ -1326,6 +1326,10 @@ export abstract class PBRBaseMaterial extends PushMaterial {
attribs.push(VertexBuffer.ColorKind);
}

if (defines.INSTANCESCOLOR) {
Copy link
Contributor

@Popov72 Popov72 Apr 23, 2022

Choose a reason for hiding this comment

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

This block of code is also missing in all materials of the material library (and maybe in some other materials, I know the fluent controls in 3D GUI are also using custom materials for eg).

I think it would be better to update the MaterialHelper.PrepareAttributesForInstances method to push VertexBuffer.ColorInstanceKind in the attribute list because this method is called each time instances/thin instances are supported by materials. This way, you won't have to update individually each material.

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.

None yet

4 participants