Exposing sky for Editor #5901
Exposing sky for Editor #5901
Conversation
src/scene/skybox/sky.js
Outdated
| @@ -59,6 +75,19 @@ class Sky { | |||
| this.projectedSkydomeCenterId = this.device.scope.resolve('projectedSkydomeCenter'); | |||
| } | |||
|
|
|||
|
|
|||
| applySettings(render) { | |||
| if (render.skyPosition) { | |||
There was a problem hiding this comment.
scale and position should be set on the this.node instead of new properties, the same way the example does it:
this.node.setLocalScale(new pc.Vec3(data.get('data.skybox.scale') ?? [1, 1, 1]));
this.node.setLocalPosition(new pc.Vec3(data.get('data.skybox.position') ?? [0, 0, 0]));
and the added position and scale properties are not needed.
There was a problem hiding this comment.
Is it too late to reconsider exposing the node property? Maybe scale, position and rotation properties would be kinda nice. Then we could just alias Scene#skyboxRotation to Sky#rotation. Having the node is only really handy if you want to do something hierarchical and that's not needed here.
There was a problem hiding this comment.
we need both rotations though. The node positions / rotates / scales the mesh itself.
but scene.skyboxRotation rotates the texture.
There was a problem hiding this comment.
I don't understand why the texture can't also be rotated with a quaternion.
There was a problem hiding this comment.
Maybe alias Scene#skyboxRotation to Scene#skyTextureRotation for more clarity then?
There was a problem hiding this comment.
maybe in a separate PR. I'd avoid doing it in this PR, this one should focus on the task that is needed.
src/scene/skybox/sky.js
Outdated
|
|
||
| applySettings(render) { | ||
| this.node.setLocalPosition(new Vec3(render.skyPosition ?? [0, 0, 0])); | ||
| this.node.setLocalScale(new Vec3(render.skyScale ?? [1, 1, 1])); |
There was a problem hiding this comment.
and for completeness, expose 'rotation' as well please - this is handy for box projection if the room geometry is not aligned with the axes.
maybe those 3 that we set on the node should be skyMeshPosition skyMeshRotation and skyMeshScale, to not confuse them with the existing skyboxRotation, that rotates the texture
There was a problem hiding this comment.
I suggested in the other comment to do skyTextureRotation for the existing rotation prop but since you said it would change the existing API this seems more logical
I think Editor could set those directly, but the other use case is when the published project is loaded. Those settings are stored in data, and are applied to the scene. Similar to how components accept loaded data to initialize. So I'm not sure we can avoid this. |
Implemenation of the engine side sky properties outlined in playcanvas/editor#1080
I confirm I have read the contributing guidelines and signed the Contributor License Agreement.