Fixed SkySphere background. #271
Conversation
| const SkySphere = ({ texturePath }: SkySphereProps) => { | ||
| const viewerState = useModelContext().viewerState; | ||
| const skyTexture = useLoader(TextureLoader, viewerState.defaultSkyTextures[viewerState.skyTextureIndex]); | ||
| const skySphereRef = useRef<Mesh>(null); |
There was a problem hiding this comment.
How does this handle the case where there's no texture and there's a background color or is that guarded against upstream?
There was a problem hiding this comment.
At this moment the SkySphere does not have an option to have a background color, only a texture.
I see two options here:
- Allowing selecting a plain color forthe skysphere instead of textures, this would be a new option for the skysphere in the tree, compiling all options for background into the same space. In this case, the skysphere will always be visible.
- Allowing making the skysphere not visible (basically removing it), and using scene/renderer background color propierties. But I am not sure where this option could live.
I think option 1 would be easier to implement and more clear, as everything related to backgrounds would be located in this class.
There was a problem hiding this comment.
I agree that option 1 would be easier but I don't know if it will allow us to create images with transparent background. If yes then I'd go for it, if not we'd need two separate options as in 4.5 where scene has a color background that would be used unless a skysphere/skybox is visible.
There was a problem hiding this comment.
The latest commit should take care of it. Now when you select skysphere on the tree, you can select if you want to use a texture of a color, and a selector appears accordingly.
For a transparent background in videos, you can now set the skybox as not visible in the tree. In the viewer this is shown as white, but in the video it will appear black. I am not sure if we want a more specific "transparent video" option for this, or if setting the visibility of the skysphere to not visible is enough.
There was a problem hiding this comment.
Thanks Alberto, the issue was reported by Carmichael as what the user sees in the app is not what they get. If we add a note somewhere about expected behavior that could do the trick, or possibly disable transparency of background for videos altogether but hard for me to assess how common these are used. Thoughts?
|
Tested in GUI and merged into it. Merging accordingly. Thanks @AlbertoCasasOrtiz 👍 Will leave open to address leftover discussion. |
I think these changes should be safe to merge into dev-gui too later.