Only update shadow properties of lights that support shadows

[mrxz]

Description:
Fixes #5747.

The light component would unconditionally update the properties of light.shadow when castShadow was enabled. However, not all light types support shadows, causing an error when switching the light type and/or enabling castShadow for an incompatible light type.

Additionally I noticed that the CameraHelper for the shadow camera would not update when changing light type. By removing the camera helper when the light type changes, it's guaranteed to be recreated for the correct camera (if needed).

Changes proposed:

• Only update light.shadow properties if the light type supports shadows
• Remove shadow camera helper when light type changes

[hazho]

I think there should be a more fundamental solution, not in the plugin/component level, but in the core of aframe, in order to make aframe a future proof and more resilient for changes and scalability.

just for example a method of schema/data properties to check if a dependent property has the use need of another dependency property, if so, proceed, otherwise skip/ignore the existence of that unnecessary property.

[mrxz]

I think there should be a more fundamental solution, not in the plugin/component level, but in the core of aframe, in order to make aframe a future proof and more resilient for changes and scalability.

A-Frame places few constraints on a component's properties. It's only concerned with parsing and equality checking, letting the component or system do whatever it wants with them. Any schema properties intended for validation (min, max, oneOf) or for conditional properties (if) have no runtime significance and currently only serve the aframe-inspector.

While it is appealing to move these responsibilities from the component into the core, that isn't as simple as it might seem. Since a component is free to do whatever it wants with the properties, there will inevitable be situations the core validation and conditional logic can't handle. Take for example validation that combines multiple properties, or validation that depends on remotely fetched resources or is otherwise asynchronous.

We should avoid a situation where validation, conditional logic and corresponding logging is split between a schema-driven part handled by the core and custom logic implemented per component.

just for example a method of schema/data properties to check if a dependent property has the use need of another dependency property, if so, proceed, otherwise skip/ignore the existence of that unnecessary property.

There is an option for components to have a "dynamic schema". This is used by the geometry and material components. It's intended use is really for components that change shape (almost) completely. While this could've been used for the light component as well, it isn't without its drawbacks.

In case the schema would only include the shadowXyz properties when castShadow is true, it would not retain or record any changes to these values while castShadow is set to false. In fact it would log warnings indicating that these properties are unknown. This means that a user simply "toggling castShadow" would find the other properties reverting to default values each time.

If I understand your suggestion correctly regarding skipping/ignoring the existence of unnecessary properties, the same behaviour would arise. Note that it'll depend on the component whether this behaviour is desired or not, which is separate from the conditional nature of the properties in question.

---

That's not to say I'm opposed to ideas exploring ways to shift (some of) the responsibility into the core. It's just that I don't see a clean way that would avoid dividing the responsibility across both or would otherwise be limited.

[dmarcos]

Thank you

[hazho]

Thanks @mrxz ,but I had tested that change, it slowed down the toggling of the castShadow props, when checking or unchecking it the INSPECTOR, I have to wait long time to be reflected, also, it consumes a lot of GPU and a bit more CPU than before this update of the code..!
also, isn't supposed to remove the extra sub-props from the light component when the castShadow unchecked? if it supposed to, then in this change of the code, it reserves the sub-props (schemas) of light comp, this same last observed when switching the light type as well

[mrxz]

Thanks @mrxz ,but I had tested that change, it slowed down the toggling of the castShadow props, when checking or unchecking it the INSPECTOR, I have to wait long time to be reflected, also, it consumes a lot of GPU and a bit more CPU than before this update of the code..!

I'm unable the reproduce this, could you provide a (live) example showing the issue?

Toggling the castShadow property does cause Three.js to compile shaders for the new lighting configuration, which can case a stutter. But this was already the case before this PR. Not to mention that Three.js caches these, so after the first toggle no such stutter should occur.

also, isn't supposed to remove the extra sub-props from the light component when the castShadow unchecked? if it supposed to, then in this change of the code, it reserves the sub-props (schemas) of light comp, this same last observed when switching the light type as well

All properties of the light component are parsed, regardless of the values of type or castShadow. It's the inspector that hides/shows properties for convenience. But this behaviour is unchanged by this PR.