View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0001914 | FSSCP | OpenGL | public | 2009-04-11 11:18 | 2009-11-19 13:29 |
Reporter | KeldorKatarn | Assigned To | Hery | ||
Priority | high | Severity | major | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.6.11 | ||||
Target Version | 3.6.12 RC1 | Fixed in Version | 3.6.12 RC1 | ||
Summary | 0001914: Skybox rendering uses texture GL_REPEAT instead of GL_CLAMP causing seams to appear. | ||||
Description | We encountered this in Saga. We recently introduced self made skyboxes instead the starfield-skysphere + FRED nebulas. We noticed visible seams between the 6 skybox textures. The reason seems to be the usage of GL_REPEAT in conjunture with small fractional float errors in the texture coordinates. We could correct for this in the shaders by simply simulating GL_CLAMP in them, but that is no solution for the fallback fixed function pipeline rendering. Since it is unlikely that tiling will be used by anyone for skyboxes, it would make sense to introduce CLAMP to avoid this issue. A test would be needed though if setting to CLAMP will be enough. I don't know if overflow by rounding error can still occur then and what that would look like. Also one has to play great attention ingame when testing. Depending on what skybox is used one might not spot this problem. if only black backrgound and stars are visible one won't see an overflow from black to black. but with planets nebulas or even a landscape skybox this will be visible to some degree or even very obvious depending on the skybox color layout. | ||||
Tags | No tags attached. | ||||
2009-05-11 19:21
|
skybox_rendering.patch (739 bytes)
Index: code/starfield/starfield.cpp =================================================================== --- code/starfield/starfield.cpp (revision 5227) +++ code/starfield/starfield.cpp (working copy) @@ -601,6 +601,7 @@ #include "parse/parselo.h" #include "hud/hud.h" #include "hud/hudtarget.h" +#include "graphics/gropengltexture.h" #define MAX_DEBRIS_VCLIPS 4 @@ -2919,8 +2920,12 @@ // draw the model at the player's eye with no z-buffering model_set_alpha(1.0f); + gr_opengl_set_texture_addressing(TMAP_ADDRESS_CLAMP); + model_render(Nmodel_num, &vmd_identity_matrix, &Eye_position, flags); + gr_opengl_set_texture_addressing(TMAP_ADDRESS_WRAP); + if (Nmodel_bitmap >= 0) model_set_forced_texture(-1); } |
|
Since nobody seems to ... Uploaded a patch. |
|
The gr_opengl_* functions should never be referenced outside of the OpenGL code without very good reason. Just use the standard function pointer instead, that's what it's there for. I still don't like this as a real fix though. While skyboxes may not make use REPEAT, skyspheres might. We have no way to know which is in use so we can't make use of both, just one or the other completely. There is a possibility that this could end up breaking another mod. I haven't really kept up with the problem either, but I have seen/used about a dozen skyboxes over the years and have never had a single issue with the tex coords. I find it difficult to believe that this is just a code issue, though I suppose it is possible (just highly unlikely). |
|
I doubt it is somehow related to the texture coordinates - DaBrain checked them as well as I did. http://www.hard-light.net/forums/index.php/topic,62783.0.html |
|
I also doubt that it's a simple texcoord issue. But the fact remains that it has worked fine historically, and is only recently having issues. Whether there is now a rendering issue (possibly shader related, anybody tested with -no_glsl?), a POF conversion/reading issue due to newer tools, of if older skyboxes actually had some strange hack which overcame the issue, I have no idea. My point is just that we can't simply treat this as an issue which is completely broken, because it did and still does work (last time I was playing with SoL at least). I'm going to go back over some older content and verify whether it still really works 100% or not, and whether there is a difference between shader/fixed rendering. If you can attach a POF and maps (doesn't have to be official content) which are known to have the problem then that would help as well. |
|
I could upload it later - I am using sky5.pof I got from SoL |
|
Allright, let's get to it. ;) This issue does occur with and without glsl. Actually, the original hack-ish solution was to counteract this in the shader. It worked, to some extent (for example seems were still visible during autopilot transitions, which use different FOV due to cutscene bars). With Keldor's latest attempt, however, seams are gone on all systems. It does not matter if shader support is enabled or not. Here is a picture taken on Radeon X1250, which has no shader support http://i278.photobucket.com/albums/kk103/limdaepl/loki3test1.jpg The skybox POF used originates from SoL, as I mentioned in my previous note. I compiled a package containing one of our skyboxes plus a set of texture maps and will send you a PM with the download link. Thanks for looking into this issue. It is very appreciated. ;) |
|
Tweaking the UVs appears to fix the seams, visually at least. Basically it's just cheating the edge blending and isn't the best solution, but it keeps the fix in the content which is less error prone than a code fix against an antiquated texture setup. This is another one of those things that would be obscenely simple to deal with if we had a material system. :( I just made the changes in code, so a proper test is needed to see if there is really enough of a difference to qualify as something that will work. See if Scooby (or whoever can do it) can modify the tex coords in the POF so that 0.0 is changed to 0.0005. It's a little backwards from how I thought that might resolve things, but it looks to work fine in my few quick tests. Oh, and you may need to delete the IBX file to make sure that it uses the new values rather than the old ones by mistake. |
|
OK, I am on it. So all you need is a skybox with 0.0005 - 0.9995 UV coordinates? |
|
I tested with 0.0005 - 1.0. Not sure if it makes that much difference. We'll see what Scooby thinks. |
|
I am not that comfortable with the solution - it sounds like a hack, to tell the truth. |
|
It's a long accepted way to get rid of skybox seams actually, you can google it if you want. Using clamp-to-edge is the other of the two possible methods to address the problem, but with our code there is simply no way that can be used properly without breaking something else. |
|
Why not simply add a property to the mission file to say whether or not to use CLAMP or REPEAT? Something like Bobb's skybox flags which I made the FRED changes for recently. http://svn.icculus.org/fs2open?view=rev&revision=5080 |
|
Would work for me... |
|
Doesn't the mediavps skybox (i.e. starfield.pof) use tiled textures, and thus require GL_REPEAT ? |
|
That's why I'd like to make this a flag you can set in FRED. It's allowing the mission designer a lot lower level access to the render code than I'd like but it's probably the best way of deciding unless we want to make it a pof feature (Which I suspect is better but would require changes to PCS II as well as the code IIRC). |
|
In the long run it would be best to make this a POF per-texture-setting feature, yes. But since this is obviously way beyond the scope of this problem right now, I'd say let's go with the flag for now, which can later be easily removed again once it is decided to implement it into POF on a larger scale. |
|
-This is a Saga showstopper- |
|
Is there a way for the code to know whether the texture is tiled or not and use the appropriate method? Then it wouldn't require any POF or mission changes. |
|
I don't think so. Then again, one could probably tie this to -wcsaga flag. It'd be a dirty solution, but an easy one. |
|
There was worry that changing the behavior could have negative side effects, but it sounds like anyone using a UV'd skybox would definitely want CLAMP, and anyone using tiling would definitely want REPEAT. Could this behavior be set based on the polymodel flag PM_FLAG_ALLOW_TILING? Oddly I can't find where in the code that actually gets set though, but it does get checked in a couple of places in modelinterp.cpp. |
|
I'm throwing this Hery's way cause I don't think he's seen his share of bugs yet :) Really though, if this isn't your thing just send it back and I'll find someone who can. Catch me in IRC if you do think you can handle it but have some questions or something. |
|
Unfortunately, PM_FLAG_ALLOW_TILING is not an instant solution to this problem. I couldn't find any model in mediavp that sets this flag (even if it wants REPEAT). Hence, if PM_FLAG_ALLOW_TILING determined texture mapping behaviour, we would completely break compatibility (probably buggy though). Anyway, it is model that has to tell the engine if it wants REPEAT or CLAMP. That's why I think the best way to solve this problem is to add new flag PM_FLAG_FORCE_CLAMP to the POF files. I'm not very familiar with the modeling and pof files, so I would be glad if someone told me whether such solution (i.e. adding new flag) has any serious drawback from the modders point of view. There might be also another possibility. The engine may check (basing on the texture coordinates) whether CLAMP or REPEAT is needed. However, currently I'm not sure if it will be possible. |
2009-11-14 13:12
|
skybox_clamp.patch (5,263 bytes)
Index: code/fred2/bgbitmapdlg.cpp =================================================================== --- code/fred2/bgbitmapdlg.cpp (revision 5657) +++ code/fred2/bgbitmapdlg.cpp (working copy) @@ -71,6 +71,7 @@ m_sky_flag_3 = The_mission.skybox_flags & MR_NO_ZBUFFER ? 1 : 0; m_sky_flag_4 = The_mission.skybox_flags & MR_NO_CULL ? 1 : 0; m_sky_flag_5 = The_mission.skybox_flags & MR_NO_GLOWMAPS ? 1 : 0; + m_sky_flag_6 = The_mission.skybox_flags & MR_FORCE_CLAMP ? 1 : 0; //}}AFX_DATA_INIT } @@ -128,6 +129,7 @@ DDX_Check(pDX, IDC_SKY_FLAG_NO_ZBUFF, m_sky_flag_3); DDX_Check(pDX, IDC_SKY_FLAG_NO_CULL, m_sky_flag_4); DDX_Check(pDX, IDC_SKY_FLAG_NO_GLOW, m_sky_flag_5); + DDX_Check(pDX, IDC_SKY_FLAG_CLAMP, m_sky_flag_6); //}}AFX_DATA_MAP } @@ -415,6 +417,9 @@ if(m_sky_flag_5) { The_mission.skybox_flags |= MR_NO_GLOWMAPS; } + if(m_sky_flag_6) { + The_mission.skybox_flags |= MR_FORCE_CLAMP; + } // close sun data sun_data_close(); Index: code/fred2/bgbitmapdlg.h =================================================================== --- code/fred2/bgbitmapdlg.h (revision 5657) +++ code/fred2/bgbitmapdlg.h (working copy) @@ -82,6 +82,7 @@ int m_sky_flag_3; int m_sky_flag_4; int m_sky_flag_5; + int m_sky_flag_6; CString m_skybox_model; CString m_envmap; //}}AFX_DATA Index: code/fred2/fred.rc =================================================================== --- code/fred2/fred.rc (revision 5657) +++ code/fred2/fred.rc (working copy) @@ -1378,6 +1378,7 @@ PUSHBUTTON "Skybox Model",IDC_SKYBOX_MODEL,228,268,78,14 CONTROL "No Lighting",IDC_SKY_FLAG_NO_LIGHTING,"Button",BS_AUTOCHECKBOX | WS_TABSTOP,239,296,52,10 CONTROL "Transparent",IDC_SKY_FLAG_XPARENT,"Button",BS_AUTOCHECKBOX | WS_TABSTOP,297,296,54,10 + CONTROL "Force clamp",IDC_SKY_FLAG_CLAMP,"Button",BS_AUTOCHECKBOX | WS_TABSTOP,351,296,54,10 CONTROL "No Z-Buffer",IDC_SKY_FLAG_NO_ZBUFF,"Button",BS_AUTOCHECKBOX | WS_TABSTOP,239,309,53,10 CONTROL "No Cull",IDC_SKY_FLAG_NO_CULL,"Button",BS_AUTOCHECKBOX | WS_TABSTOP,297,309,39,10 CONTROL "No Glowmaps",IDC_SKY_FLAG_NO_GLOW,"Button",BS_AUTOCHECKBOX | WS_TABSTOP,351,309,60,10 Index: code/fred2/resource.h =================================================================== --- code/fred2/resource.h (revision 5657) +++ code/fred2/resource.h (working copy) @@ -1119,6 +1119,7 @@ #define IDC_SKY_FLAG_NO_ZBUFF 1611 #define IDC_SKY_FLAG_NO_CULL 1612 #define IDC_SKY_FLAG_NO_GLOW 1613 +#define IDC_SKY_FLAG_CLAMP 1614 #define IDC_TURRETS_LOCKED 1615 #define IDC_AFTERBURNER_LOCKED 1616 #define IDC_FORCE_SHIELDS 1617 Index: code/lab/lab.cpp =================================================================== --- code/lab/lab.cpp (revision 5657) +++ code/lab/lab.cpp (working copy) @@ -1638,6 +1638,7 @@ ADD_RENDER_FLAG("No Lighting", Lab_model_flags, MR_NO_LIGHTING); ADD_RENDER_FLAG("No Z-Buffer", Lab_model_flags, MR_NO_ZBUFFER); ADD_RENDER_FLAG("No Culling", Lab_model_flags, MR_NO_CULL); + ADD_RENDER_FLAG("Force Clamp", Lab_model_flags, MR_FORCE_CLAMP); ADD_RENDER_FLAG("Show Full Detail", Lab_model_flags, MR_FULL_DETAIL); ADD_RENDER_FLAG("Show Pivots", Lab_model_flags, MR_SHOW_PIVOTS); ADD_RENDER_FLAG("Show Paths", Lab_model_flags, MR_SHOW_PATHS); Index: code/model/model.h =================================================================== --- code/model/model.h (revision 5657) +++ code/model/model.h (working copy) @@ -762,6 +762,7 @@ #define MR_SHOW_OUTLINE_HTL (1<<26) // Show outlines (wireframe view) using HTL method #define MR_NO_GLOWMAPS (1<<27) // disable rendering of glowmaps - taylor #define MR_FULL_DETAIL (1<<28) // render all valid objects, particularly ones that are otherwise in/out of render boxes - taylor +#define MR_FORCE_CLAMP (1<<29) // force clamp - Hery // old/obsolete flags //#define MR_SHOW_DAMAGE (1<<4) // Show the "destroyed" subobjects Index: code/model/modelinterp.cpp =================================================================== --- code/model/modelinterp.cpp (revision 5657) +++ code/model/modelinterp.cpp (working copy) @@ -2375,6 +2375,8 @@ model_do_dumb_rotation(model_num); + if (flags & MR_FORCE_CLAMP) + gr_screen.gf_set_texture_addressing(TMAP_ADDRESS_CLAMP); int time = timestamp(); for (int i = 0; i < pm->n_glow_point_banks; i++ ) { //glow point blink code -Bobboau @@ -2478,6 +2480,9 @@ if(The_mission.flags & MISSION_FLAG_FULLNEB){ gr_fog_set(GR_FOGMODE_NONE, 0, 0, 0); } + + if (flags & MR_FORCE_CLAMP) + gr_screen.gf_set_texture_addressing(TMAP_ADDRESS_WRAP); } /* Index: code/model/modelread.cpp =================================================================== --- code/model/modelread.cpp (revision 5657) +++ code/model/modelread.cpp (working copy) @@ -44,7 +44,8 @@ {"transparent", MR_ALL_XPARENT}, {"no Zbuffer", MR_NO_ZBUFFER}, {"no cull", MR_NO_CULL}, - {"no glowmaps", MR_NO_GLOWMAPS} + {"no glowmaps", MR_NO_GLOWMAPS}, + {"force clamp", MR_FORCE_CLAMP}, }; int model_render_flags_size = sizeof(model_render_flags)/sizeof(flag_def_list); |
|
I've added new flag in FRED "Force Clamp" that forces skybox texture to be clamped instead of repeated. It is possible to use this texture also in other models. Debug build is available here: http://www.quarnos.org/dl/skybox_build.zip Please, confirm that the problem is solved. |
|
Sounds like a good solution. I'll ask Keldor to disable clamp in our shader set - personally I have no idea where to start looking... ;) |
|
chief1983 committed my patch to the trunk. |
Date Modified | Username | Field | Change |
---|---|---|---|
2009-04-11 11:18 | KeldorKatarn | New Issue | |
2009-05-11 19:21 | KeldorKatarn | File Added: skybox_rendering.patch | |
2009-05-11 19:22 | KeldorKatarn | Note Added: 0010882 | |
2009-05-12 00:11 | taylor | Note Added: 0010883 | |
2009-05-12 10:28 | Tolwyn | Note Added: 0010884 | |
2009-05-12 20:06 | taylor | Note Added: 0010885 | |
2009-05-12 20:36 | Tolwyn | Note Added: 0010886 | |
2009-05-13 06:39 | Tolwyn | Note Added: 0010891 | |
2009-05-13 08:01 | taylor | Note Added: 0010892 | |
2009-05-13 09:43 | Tolwyn | Note Added: 0010893 | |
2009-05-13 09:49 | taylor | Note Added: 0010894 | |
2009-05-13 10:55 | Tolwyn | Note Added: 0010896 | |
2009-05-13 15:18 | taylor | Note Added: 0010898 | |
2009-05-18 22:25 | karajorma | Note Added: 0010909 | |
2009-06-20 20:06 | KeldorKatarn | Note Added: 0010984 | |
2009-06-24 00:14 | Aardwolf | Note Added: 0010986 | |
2009-06-24 06:39 | karajorma | Note Added: 0010990 | |
2009-06-24 16:17 | KeldorKatarn | Note Added: 0010991 | |
2009-06-26 17:00 | KeldorKatarn | Note Added: 0010996 | |
2009-07-29 20:42 | chief1983 | Note Added: 0011117 | |
2009-07-29 20:48 | Tolwyn | Note Added: 0011118 | |
2009-07-30 16:55 | chief1983 | Note Added: 0011119 | |
2009-11-10 03:35 | chief1983 | Note Added: 0011233 | |
2009-11-10 03:35 | chief1983 | Assigned To | => Hery |
2009-11-10 03:35 | chief1983 | Priority | normal => high |
2009-11-10 03:35 | chief1983 | Status | new => assigned |
2009-11-10 03:35 | chief1983 | Product Version | 3.6.9 => 3.6.11 |
2009-11-10 03:35 | chief1983 | Target Version | => 3.6.12 RC1 |
2009-11-11 13:20 | Hery | Note Added: 0011245 | |
2009-11-11 13:30 | Hery | Note Edited: 0011245 | |
2009-11-14 13:12 | Hery | File Added: skybox_clamp.patch | |
2009-11-14 13:14 | Hery | Note Added: 0011279 | |
2009-11-14 13:33 | Tolwyn | Note Added: 0011280 | |
2009-11-19 13:29 | Hery | Note Added: 0011307 | |
2009-11-19 13:29 | Hery | Status | assigned => resolved |
2009-11-19 13:29 | Hery | Fixed in Version | => 3.6.12 RC1 |
2009-11-19 13:29 | Hery | Resolution | open => fixed |