-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: add reflector #136
feat: add reflector #136
Conversation
✅ Deploy Preview for cientos-tresjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @JaimeTorrealba excellent job, I love the result.
I think we should add this to abstractions instead of materials because this implementation actually doesn't solve #5, it's not a Material but I see it as a handy abstraction from 'three/addons/objects/Reflector'
The #5 is an actual material, to be used like this:
<TresMesh >
<TresPlaneGeometry />
<MeshReflectorMaterial blur="[300, 100]" resolution="2048" />
</TresMesh>
Another question would be, how are you handling the reactivity of those props?
@alvarosabu could be related to this doc's problem #137 |
In relation to the other comment, You're right is not a material, I'll move it, but It solves the exact same problem of the #5. As the properties are in the args (And because of the way the Reflector class have been built in, I couldn't figure out how to make reactive props) only the color is reactive |
For the ticket leave it open, the idea would be to eventually create te reflector material. For the reactivity I give you a small clue since you can get the reference of the instance, if one of the argument changes (watch) you could potentially dispose it and re initialize it with the new args. Give it a though 😉 |
I tried already but nothing work, nor dispose the material, geometry. Nothing works, I Guess I am doing something wrong :( |
@JaimeTorrealba can you merge latest main and update this PR, it has been around for long |
@alvarosabu I already merge main in this one. Is not ready yet for merging to main, because is not reactive, when you have some time check it please. If you have any doubts, please let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Here are my suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @JaimeTorrealba awesome to see this updated, can you add a demo to the documentation, this one is especially visual, it would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JaimeTorrealba Thanks for this, looks amazing, great job
No description provided.