2019-12-07 11:56 EST


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0001914FSSCPOpenGLpublic2009-11-19 08:29
ReporterKeldorKatarn 
Assigned ToHery 
PriorityhighSeveritymajorReproducibilityalways
StatusresolvedResolutionfixed 
Product Version3.6.11 
Target Version3.6.12 RC1Fixed in Version3.6.12 RC1 
Summary0001914: Skybox rendering uses texture GL_REPEAT instead of GL_CLAMP causing seams to appear.
DescriptionWe 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.
TagsNo tags attached.
Attached Files
  • patch file icon skybox_rendering.patch (739 bytes) 2009-05-11 15:21 -
    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);
     }
    
    patch file icon skybox_rendering.patch (739 bytes) 2009-05-11 15:21 +
  • patch file icon skybox_clamp.patch (5,263 bytes) 2009-11-14 08:12 -
    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);
    
    patch file icon skybox_clamp.patch (5,263 bytes) 2009-11-14 08:12 +

-Relationships
+Relationships

-Notes

~0010882

KeldorKatarn (reporter)

Since nobody seems to ...
Uploaded a patch.

~0010883

taylor (administrator)

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).

~0010884

Tolwyn (reporter)

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

~0010885

taylor (administrator)

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.

~0010886

Tolwyn (reporter)

I could upload it later - I am using sky5.pof I got from SoL

~0010891

Tolwyn (reporter)

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. ;)

~0010892

taylor (administrator)

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.

~0010893

Tolwyn (reporter)

OK, I am on it.

So all you need is a skybox with 0.0005 - 0.9995 UV coordinates?

~0010894

taylor (administrator)

I tested with 0.0005 - 1.0. Not sure if it makes that much difference. We'll see what Scooby thinks.

~0010896

Tolwyn (reporter)

I am not that comfortable with the solution - it sounds like a hack, to tell the truth.

~0010898

taylor (administrator)

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.

~0010909

karajorma (administrator)

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

~0010984

KeldorKatarn (reporter)

Would work for me...

~0010986

Aardwolf (reporter)

Doesn't the mediavps skybox (i.e. starfield.pof) use tiled textures, and thus require GL_REPEAT ?

~0010990

karajorma (administrator)

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).

~0010991

KeldorKatarn (reporter)

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.

~0010996

KeldorKatarn (reporter)

-This is a Saga showstopper-

~0011117

chief1983 (administrator)

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.

~0011118

Tolwyn (reporter)

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.

~0011119

chief1983 (administrator)

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.

~0011233

chief1983 (administrator)

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.

~0011245

Hery (developer)

Last edited: 2009-11-11 08:30

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.

~0011279

Hery (developer)

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.

~0011280

Tolwyn (reporter)

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... ;)

~0011307

Hery (developer)

chief1983 committed my patch to the trunk.
+Notes

-Issue History
Date Modified Username Field Change
2009-04-11 07:18 KeldorKatarn New Issue
2009-05-11 15:21 KeldorKatarn File Added: skybox_rendering.patch
2009-05-11 15:22 KeldorKatarn Note Added: 0010882
2009-05-11 20:11 taylor Note Added: 0010883
2009-05-12 06:28 Tolwyn Note Added: 0010884
2009-05-12 16:06 taylor Note Added: 0010885
2009-05-12 16:36 Tolwyn Note Added: 0010886
2009-05-13 02:39 Tolwyn Note Added: 0010891
2009-05-13 04:01 taylor Note Added: 0010892
2009-05-13 05:43 Tolwyn Note Added: 0010893
2009-05-13 05:49 taylor Note Added: 0010894
2009-05-13 06:55 Tolwyn Note Added: 0010896
2009-05-13 11:18 taylor Note Added: 0010898
2009-05-18 18:25 karajorma Note Added: 0010909
2009-06-20 16:06 KeldorKatarn Note Added: 0010984
2009-06-23 20:14 Aardwolf Note Added: 0010986
2009-06-24 02:39 karajorma Note Added: 0010990
2009-06-24 12:17 KeldorKatarn Note Added: 0010991
2009-06-26 13:00 KeldorKatarn Note Added: 0010996
2009-07-29 16:42 chief1983 Note Added: 0011117
2009-07-29 16:48 Tolwyn Note Added: 0011118
2009-07-30 12:55 chief1983 Note Added: 0011119
2009-11-09 22:35 chief1983 Note Added: 0011233
2009-11-09 22:35 chief1983 Assigned To => Hery
2009-11-09 22:35 chief1983 Priority normal => high
2009-11-09 22:35 chief1983 Status new => assigned
2009-11-09 22:35 chief1983 Product Version 3.6.9 => 3.6.11
2009-11-09 22:35 chief1983 Target Version => 3.6.12 RC1
2009-11-11 08:20 Hery Note Added: 0011245
2009-11-11 08:30 Hery Note Edited: 0011245
2009-11-14 08:12 Hery File Added: skybox_clamp.patch
2009-11-14 08:14 Hery Note Added: 0011279
2009-11-14 08:33 Tolwyn Note Added: 0011280
2009-11-19 08:29 Hery Note Added: 0011307
2009-11-19 08:29 Hery Status assigned => resolved
2009-11-19 08:29 Hery Fixed in Version => 3.6.12 RC1
2009-11-19 08:29 Hery Resolution open => fixed
+Issue History