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

generic type for shaderMaterial, exposing uniform props #1602

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

xinaesthete
Copy link

@xinaesthete xinaesthete commented Aug 5, 2023

Why

I and at least one other person were having trouble with the type of shaderMaterial, as discussed in this StackOverflow question.

Actually, as of this writing I'm not really entirely sure what the relationship to useRef is, but this seems like the right approach for the returned type of shaderMaterial.

What

I've added a type parameter to shaderMaterial, inferred from the type of the uniforms argument, and intersected that with the returned material type. As such, the type of the returned value should reflect the fact that each of the uniforms is available as a prop of appropriate type.

Checklist

  • Documentation updated (example)
  • Storybook entry added (example)
  • Ready to be merged

@vercel
Copy link

vercel bot commented Aug 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
drei ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 9, 2023 11:30am

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 9, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit af564ee:

Sandbox Source
nervous-waterfall-wjhryw Configuration
Ground reflections and video textures Configuration
arc-x-pmndrs-colors Configuration

@xinaesthete
Copy link
Author

So when I saw failing CI stuff before I couldn't see the actual errors and I wasn't sure if it was to do with actual problems in the actual code vs not running properly because of who I was or something.

I realised the thing that was failing was an existing use of shaderMaterial in MeshReflectionMaterial, where the actual original call to shaderMaterial had envMap: null, ..., resolution: THREE.Vector2 which then were incompatible with their use...

So I still think the change I made in the PR is basically a good one - I'd certainly prefer to have this type inferred, and for that to - but there could be user-code that would get broken in a similar way if it got through to a release.

I'm also not entirely sure that the way I fixed issues with existing code in my last commit is necessarily the best - still had some niggles with getting it .

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

1 participant