View Issue Details

IDProjectCategoryView StatusLast Update
0001914FSSCPOpenGLpublic2009-11-19 13:29
ReporterKeldorKatarn Assigned ToHery  
PriorityhighSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
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.

Activities

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);
 }
skybox_rendering.patch (739 bytes)   

KeldorKatarn

2009-05-11 19:22

reporter   ~0010882

Since nobody seems to ...
Uploaded a patch.

taylor

2009-05-12 00:11

administrator   ~0010883

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

Tolwyn

2009-05-12 10:28

reporter   ~0010884

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

taylor

2009-05-12 20:06

administrator   ~0010885

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.

Tolwyn

2009-05-12 20:36

reporter   ~0010886

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

Tolwyn

2009-05-13 06:39

reporter   ~0010891

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

taylor

2009-05-13 08:01

administrator   ~0010892

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.

Tolwyn

2009-05-13 09:43

reporter   ~0010893

OK, I am on it.

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

taylor

2009-05-13 09:49

administrator   ~0010894

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

Tolwyn

2009-05-13 10:55

reporter   ~0010896

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

taylor

2009-05-13 15:18

administrator   ~0010898

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.

karajorma

2009-05-18 22:25

administrator   ~0010909

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

KeldorKatarn

2009-06-20 20:06

reporter   ~0010984

Would work for me...

Aardwolf

2009-06-24 00:14

reporter   ~0010986

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

karajorma

2009-06-24 06:39

administrator   ~0010990

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

KeldorKatarn

2009-06-24 16:17

reporter   ~0010991

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.

KeldorKatarn

2009-06-26 17:00

reporter   ~0010996

-This is a Saga showstopper-

chief1983

2009-07-29 20:42

administrator   ~0011117

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.

Tolwyn

2009-07-29 20:48

reporter   ~0011118

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.

chief1983

2009-07-30 16:55

administrator   ~0011119

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.

chief1983

2009-11-10 03:35

administrator   ~0011233

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.

Hery

2009-11-11 13:20

developer   ~0011245

Last edited: 2009-11-11 13: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.

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);
skybox_clamp.patch (5,263 bytes)   

Hery

2009-11-14 13:14

developer   ~0011279

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.

Tolwyn

2009-11-14 13:33

reporter   ~0011280

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

Hery

2009-11-19 13:29

developer   ~0011307

chief1983 committed my patch to the trunk.

Issue History

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