feat(app): Add shadow camera helpers to v-light-helper #337
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.
Hey @JaimeTorrealba !
I've found some bugs and made a few suggestions for getting rid of them.
There are also a few changes/suggestions for typos, etc.
😎
|
|
||
| ::: warning | ||
| This directive just work with the following lights:DirectionalLight,PointLight, SpotLight, HemisphereLight. | ||
| By this way you can't tweaks the props for the helper if you need to do that, please use the normal helper instance | ||
| This directive just work with the following lights:DirectionalLight,PointLight, SpotLight, HemisphereLight and RectAreaLight. |
There was a problem hiding this comment.
Typo and maybe wording(?)
This directive just work
"just works" is fine, but it's ambiguous in this particular context. It can mean either of these:
- "It accomplishes the task without your intervention, if you use the following: ..."
- "It only works with the following:"
I think it means the second option here, so:
This directive only works
Spacing
lights:DirectionalLight,PointLight,
lights: DirectionalLight, PointLight,
| This directive just work with the following lights:DirectionalLight,PointLight, SpotLight, HemisphereLight. | ||
| By this way you can't tweaks the props for the helper if you need to do that, please use the normal helper instance | ||
| This directive just work with the following lights:DirectionalLight,PointLight, SpotLight, HemisphereLight and RectAreaLight. | ||
| By this way you can't tweaks the props of the helper, if you need to do that, please use the normal helper instance |
There was a problem hiding this comment.
In English, I think we don't normally say "by this way", though I found a few instances online. It's usually "in this way", at least for me. (Prepositions are tough and don't make logical sense!)
But I think we can just drop it here:
By this way you can't tweaks the props of
You can't tweaks the props of
Also, this is a "comma splice":
the helper, if you need to do that
the helper. If you need to do that
| @@ -26,8 +26,29 @@ import { OrbitControls, Sphere, vLightHelper } from '@tresjs/cientos' | |||
| </TresCanvas> | |||
| </template> | |||
| ``` | |||
| ### shadowCamera | |||
|
|
|||
| Using the `shadowCamera` argument you can also instantiate the helper for the current `shadow-camera` this only works in lights that generate shadows. | |||
There was a problem hiding this comment.
shadow-camerathis only works
shadow-camera. This only works
| v-light-helper:shadowCamera | ||
| /> | ||
| <TresHemisphereLight | ||
| v-light-helper:shadowCamera // This won't work, hemisphereLight doesn't generate shadows |
There was a problem hiding this comment.
This won't work,
This won't work.
| </script> | ||
|
|
||
| <template> | ||
| <TresLeches |
There was a problem hiding this comment.
There seems to be a bug related to removing the Light/Helper.
When any of these Leches boxes are unticked – removing the light helper – errors are being logged to the console for me:
It shows up in the latest FF and Chrome on Mac, anyway.
| import { RectAreaLightHelper } from 'three-stdlib' | ||
|
|
||
| const { logWarning } = useLogger() | ||
|
|
||
| export const vLightHelper = { | ||
| mounted: (el: any) => { | ||
| mounted: (el: any, binding: any) => { |
There was a problem hiding this comment.
It looks like el could be typed as a Light, given the usage here. That will allow the type checker to highlight a few problems down below.
| @@ -40,40 +52,45 @@ onLoop(({ elapsed }) => { | |||
| </Sphere> | |||
| <TresAmbientLight :color="0xffffff" /> | |||
| <TresDirectionalLight | |||
There was a problem hiding this comment.
If this is placed in a group, an error is thrown.
<TresGroup>
<TresDirectionalLight
v-if="dirLight.value"
ref="directionalLightRef"
v-light-helper:shadowCamera
:color="0xffffff"
:intensity="5"
:position="[0, 2, 4]"
/>
</TresGroup>Uncaught (in promise) TypeError: currentShadowCameraHelper is undefined
| const getHelperIntance = (el: any) => | ||
| el.parent.children.find((child: any) => child instanceof currentHelper) | ||
| const getShadowCameraHelperIntance = (el: any) => | ||
| el.parent.children.find((child: any) => child instanceof CameraHelper) |
There was a problem hiding this comment.
I think this approach is causing some problems. If you el.parent.children.find there might be multiple instances of a particular instanceof, if there are sibling lights.
Suggestion
LightHelpers and CameraHelpers store a reference to their targets. You could match on those.
| }, | ||
| } | ||
| let currentHelper: any | ||
| let currentInstance: any | ||
| let currentShadowCameraHelper: any |
There was a problem hiding this comment.
I think these variables are out of sync when the code reaches line 29. If I understand correctly, the code could lead to this incorrect flow:
- Create SpotLight 1, add helper
- Create PointLight 2, add helper
- updated() on SpotLight 1 <--
getHelperInstancewill return the PointLight helper instance, but should return the SpotLight helper instance
Suggestion
If you want to store references, maybe something like this?
type LightHelper = DirectionalLightHelper | PointLightHelper | SpotLightHelper | HemisphereLightHelper | RectAreaLightHelper
const lightUuidToShadowCameraHelper: Record<string, CameraHelper> = {}
const lightUuidToLightHelper: Record<string, LightHelper> = {}
|
@andretchen0 thanks for taking the time, I like your feedback. I'm going to put this in draw because we're moving the directive section to the core pkg. So I don't know yet what will happen with this one |
|
I'll close this one, since now the directives are in the core (I'll re-open there) |
No description provided.