BGE: Skybox
AbandonedPublic
Actions

Authored by Thomas Szepe (hg1) on Aug 12 2015, 7:17 PM.

Details

Reviewers

Sybren A. Stüvel (sybren)
Porteries Tristan (panzergame)
Daniel Stokes (kupoman)
Jorge Bernal (lordloki)
Antonis Ryakiotakis (psy-fi)
Mitchell Stokes (moguri)

Summary

This patch adds a skcybox rendering to the BGE and the 3D-Viewport.
The Skybox is working in multitexture and GLSL mode.
If the skybox is enabled in the world settings, the first 6 world texture slots are used for it.

The initial patch done by Martin Sell (Moerdn).

Diff Detail

Event Timeline

Thomas Szepe (hg1) updated this revision to Diff 4848.Aug 12 2015, 7:17 PM

Thomas Szepe (hg1) retitled this revision from to BGE: Skybox.

Thomas Szepe (hg1) updated this object.

Thomas Szepe (hg1) added projects: Game Engine, Game Rendering, Game UI, GPU / Viewport.

Test file:

Skybox.blend3 MBDownload

Changes since Moerden's initial version.

Update for actual master.
Fix crash if no world exist.
Move duplicate render code to gpu_draw.c.
Invert bottom texture vertical.

Planed improvements.

Displaylist for skybox rendering.
Use GL_CLAMP_TO_EDGE for texture coordinates.
Disable skybox in orthographic view.

There some points are open for me.

Do we need features like invert horizontal, vertical and rotate for the image? Or should the user do this with an image program?
Is it really good to use the 6 world texture slots for this or should the textures assigned in a own texture slot window or should be tried to reuse the existing Background images function?
Actually the existing background rendering only works if you look in orthographic view strait to the view. Is it better to try to modify the existing background rendering function, that it render also in perspective on all angels?
Should we support single texture texture cubemaps with different alignments. And when which one?
- Blender
- Horizontal (Cross OpenGL)
- Horizontal right
- Horizontal T (Terragen, Halo)
- Horizontal T right (Tensil, Source Engine)
- Vertical Cross (ATI CubeMapGen)
- Horizontal Strip
- Horizontal Strip (Nvidia DDS Exporter Layout)
- Vertical Stripe or column
Should the skybox disabled in orthographic view or should it rendered and scaled up by an value in the UI.
Should GLSL use a shader for the skybox rendering?

source/gameengine/Rasterizer/RAS_OpenGLRasterizer/RAS_OpenGLRasterizer.cpp
123	I forgotten to change this.

Porteries Tristan (panzergame) added inline comments.Aug 12 2015, 9:24 PM

source/gameengine/Ketsji/KX_WorldInfo.cpp
73	you can use && operator, when one of condition is wrong the check is stopped else we check the next condition : if (blenderscene->world->mtex[i] && blenderscene->world->mtex[i]->tex && blenderscene->world->mtex[i]->tex->ima)

Mitchell Stokes (moguri) added reviewers: Daniel Stokes (kupoman), Antonis Ryakiotakis (psy-fi).Aug 12 2015, 10:21 PM

It works well, nicely done.

In D1461#30762, @Thomas Szepe (hg1) wrote:

Do we need features like invert horizontal, vertical and rotate for the image? Or should the user do this with an image program?

For this patch, I wouldn't add those -- you can always add such functionality in the future.

Is it really good to use the 6 world texture slots for this or should the textures assigned in a own texture slot window or should be tried to reuse the existing Background images function?

I think world texture slots are fine, but I also feel that this should be discussed well before diving into the implementation. Maybe include the UI team in the discussion?

Maybe you could update the texture UI when the "Sky box" checkbox is enabled, so that it's more intuitive to use. Right now people don't know they have to switch to the textures tab, and once there they don't know they have to enable the world textures, and once there they don't know how many textures (and in which order) they have to put in.

Should we support single texture texture cubemaps with different alignments. And when which one?

Blender

Horizontal (Cross OpenGL)

Horizontal right

Horizontal T (Terragen, Halo)

Horizontal T right (Tensil, Source Engine)

Vertical Cross (ATI CubeMapGen)

Horizontal Strip

Horizontal Strip (Nvidia DDS Exporter Layout)

Vertical Stripe or column

I would welcome single-texture skyboxes, as it makes things easier to manage file-wise. Clicking one file is easier than clicking 6. However, requiring an image per side of the cube does give you a good answer to which alignment to support: none. Clicking 6 files can also be resolved by auto-detecting common naming standards, and loading all 6 sides into the correct slots.

Code-wise, I have an issue with the numbers 0-5. There is no indication what so ever as to which number belongs to which side. To determine this, you have to dive into the big switch-statement, decompile the math used in there, and hunt for the differences between the cases. This should be improved in the code itself (the switch can be heavily simplified) and in documentation (both in user documentation and code comments). Of course I understand that you'll only write docs after the code itself is finalized ;-)

source/blender/editors/space_view3d/view3d_draw.c
3609	I would store `scene->world->mtex[iter]` in a temporary variable, to avoid repeating array lookups and improve readability, and short-cut using `&&` operators.
3800	Use `(scene->world && (scene->world->flag & WO_DS_SHOW_SKYBOX))` here, instead of two nested if-statements.
source/blender/gpu/intern/gpu_draw.c
2071	What does `iter` do? It seems to choose a side of the sky cube, so why isn't it called `cube_side` instead?
2081	It's very difficult to see the differences between the different cases. It may be better to define some variables, give them their values based on `iter`, then use those variables in the glXXX calls. Use the appropriate code style.
source/blender/makesrna/intern/rna_world.c
485	Why does this patch contain changes to mist-specific code? In what way is this required to get the skybox to work?
519	Replace "texture Skydome" with "textured skybox" -- a dome is spherical.
source/gameengine/Ketsji/KX_WorldInfo.cpp
73	It's probably also better to negate the condition, and use a `continue` statement, like above.
103	The function name should be plural (`getSkyBoxTextureIds`) as multiple IDs are returned. Is there a reason that you use `int *sides` here instead of `int sides[6]`?
200	Why use `getSkyBoxTextureId(tex)` here, instead of just using `m_skyboxtex`? It seems like an unnecessary copy. If the copy is necessary, please add a comment to indicate why.
source/gameengine/Rasterizer/RAS_OpenGLRasterizer/RAS_OpenGLRasterizer.cpp
251	Code style: space after `for` (also in other places).
source/gameengine/Rasterizer/RAS_OpenGLRasterizer/RAS_OpenGLRasterizer.h
198	Please add a comment as to what this method does. The `sides` pointer must point to a large enough block of data, or it'll crash -- such cases should be documented.

Campbell Barton (campbellbarton) removed a reviewer: Campbell Barton (campbellbarton).Aug 11 2016, 6:18 AM

This patch is still valid since world rendering with cube map was implemented ?

As I know the BGE use the cube map only to render object refections and still not render a background cube map. Actually Blender only render it in the viewport but not in the BGE.
I think you're confusing that with UPBGE where world background rendering is working.

I thought I will keep this open as long it is not implemented. But if you think it is better to close it, I will do it.