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

THREE material defines can't be set via Tres props #605

Open
5 tasks done
andretchen0 opened this issue Mar 28, 2024 · 0 comments
Open
5 tasks done

THREE material defines can't be set via Tres props #605

andretchen0 opened this issue Mar 28, 2024 · 0 comments
Labels
bug Something isn't working p2-edge-case Bug, but has a workaround or limited in scope (priority)

Comments

@andretchen0
Copy link
Contributor

andretchen0 commented Mar 28, 2024

Expected

<MyMaterial defines-STANDARD="myValue" />

... should patch defines.STANDARD.

Bug

It patches defines.standard.

Cause

When patching, src/nodeOps.ts lowercases pierced props.

finalKey = key.toLowerCase()

Context

THREE materials have a defines property. It's an object whose key/value pairs are injected into the material's shader during compilation.

By convention, the keys are written in ALL_CAPS case. This is the case for all THREE materials and for many external materials for use in the THREE ecosystem.

For example, here's MeshStandardMaterial's defines:

                this.defines = { 'STANDARD': '' };

https://github.com/mrdoob/three.js/blob/ef80ac74e6716a50104a57d8add6c8a950bff8d7/src/materials/MeshStandardMaterial.js#L73

Workaround

I'm working on porting a material to Cientos. It extends THREE.MeshStandardMaterial and adds 3 defines. The extended material follows THREE's convention and uses ALL_CAPS case for the keys.

The workaround I'm currently using modifies the material's source. That would otherwise be unnecessary and means that if the original material is updated in the future, we can't simply grab a copy and insert it into Tres.

This is possible in this case, because I'm extending MeshStandardMaterial. However, under the current Tres setup, one can't meaningfully use :defines-X as a prop on built-in materials, as far as I can see.

Reproduction

https://stackblitz.com/edit/tresjs-basic-akdmgw?file=src%2Fcomponents%2FTheExperience.vue

Steps to reproduce

See StackBlitz.

System Info

All systems affected

Used Package Manager

npm

Code of Conduct

@andretchen0 andretchen0 added bug Something isn't working p2-edge-case Bug, but has a workaround or limited in scope (priority) labels Mar 28, 2024
@andretchen0 andretchen0 changed the title Lowercasing pierced props means THREE material/shader defines can't be set via Tres props. THREE material defines can't be set via Tres props Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2-edge-case Bug, but has a workaround or limited in scope (priority)
Projects
None yet
Development

No branches or pull requests

1 participant