View Issue Details

IDProjectCategoryView StatusLast Update
0003076FSSCPgraphicspublic2014-07-22 01:09
ReporterMageKing17 Assigned ToMageKing17  
PrioritylowSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Target Version3.7.2Fixed in Version3.7.2 
Summary0003076: Animated textures in a skybox behave counter-intuitively
Descriptionmodel_interp_get_texture() uses game_get_overall_frametime() as the basis for animated textures. For ships, this isn't really a problem; it subtracts the timestamp of the ship's creation. For anything that isn't a ship, this timestamp is treated as 0... resulting in weirdness whereby animated textures in skyboxes (for example) start at a seemingly-random point, not based on when you started of the mission or when the skybox model was switched to.
Steps To ReproduceAxem has helpfully created a test mod that demonstrates the issue: http://www.mediafire.com/download/qsi2phgt5982be0/SillySkybox.rar
TagsNo tags attached.

Activities

MageKing17

2014-07-12 16:04

developer   ~0016068

New version of the patch determines if a skybox is being rendered with a new MR_SKYBOX flag (this was considered preferable to Yet Another State-Tracking Global Variable). In order for model_interp_tmappoly() to get access to this flag, several functions got flags arguments added. On the plus side, the test mod still works, so this should be ready for code review.

m_m

2014-07-15 10:37

developer   ~0016072

Looks good to me but maybe someone who knows more about the model rendering than me could also take a look at it.
This might also break compatibility with missions that use the flags on the set-skybox-model SEXP but as those were only added recently and are not in any released FSO build it should be fine.

The_E

2014-07-15 12:32

administrator   ~0016073

Why did you add MR_SKYBOX to the default flags?

MageKing17

2014-07-15 15:34

developer   ~0016074

DEFAULT_NMODEL_FLAGS are used for the Skybox only. Specifically, the skybox model number is the global "Nmodel_num".

MageKing17

2014-07-15 20:52

developer  

animated textures 3.patch (6,973 bytes)   
Index: code/freespace2/freespace.cpp
===================================================================
--- code/freespace2/freespace.cpp	(revision 10918)
+++ code/freespace2/freespace.cpp	(working copy)
@@ -1025,6 +1025,7 @@
 	control_config_clear_used_status();
 	collide_ship_ship_sounds_init();
 	Missiontime = 0;
+	Skybox_timestamp = game_get_overall_frametime();
 	Pre_player_entry = 1;			//	Means the player has not yet entered.
 	Entry_delay_time = 0;			//	Could get overwritten in mission read.
 	observer_init();
Index: code/freespace2/freespace.h
===================================================================
--- code/freespace2/freespace.h	(revision 10918)
+++ code/freespace2/freespace.h	(working copy)
@@ -37,6 +37,7 @@
 extern float flFrametime;
 extern fix Missiontime;
 extern int Last_frame_timestamp; // A timestamp for when the previous frame ended
+extern fix Skybox_timestamp;	// A timestamp for animated skyboxes -MageKing17
 
 // 0 - 4
 extern int Game_skill_level;
Index: code/globalincs/systemvars.cpp
===================================================================
--- code/globalincs/systemvars.cpp	(revision 10918)
+++ code/globalincs/systemvars.cpp	(working copy)
@@ -17,6 +17,7 @@
 
 
 fix Missiontime;
+fix Skybox_timestamp;
 fix Frametime;
 int	Framecount=0;
 
Index: code/globalincs/systemvars.h
===================================================================
--- code/globalincs/systemvars.h	(revision 10918)
+++ code/globalincs/systemvars.h	(working copy)
@@ -80,6 +80,7 @@
 } vci;
 
 extern fix Missiontime;
+extern fix Skybox_timestamp;
 extern fix Frametime;
 extern int Framecount;
 
Index: code/model/model.h
===================================================================
--- code/model/model.h	(revision 10918)
+++ code/model/model.h	(working copy)
@@ -846,7 +846,7 @@
 
 // Renders a model and all it's submodels.
 // See MR_? defines for values for flags
-void model_render(int model_num, matrix *orient, vec3d * pos, uint flags = MR_NORMAL, int objnum = -1, int lighting_skip = -1, int *replacement_textures = NULL);
+void model_render(int model_num, matrix *orient, vec3d * pos, uint flags = MR_NORMAL, int objnum = -1, int lighting_skip = -1, int *replacement_textures = NULL, const bool is_skybox = false);
 
 // Renders just one particular submodel on a model.
 // See MR_? defines for values for flags
Index: code/model/modelinterp.cpp
===================================================================
--- code/model/modelinterp.cpp	(revision 10918)
+++ code/model/modelinterp.cpp	(working copy)
@@ -761,15 +761,6 @@
 	texture_info *tglow = &tmap->textures[TM_GLOW_TYPE];
 	int rt_begin_index = tmap_num*TM_NUM_TYPES;
 
-	// Goober5000
-	Interp_base_frametime = 0;
-	if (Interp_objnum >= 0)
-	{
-		object *objp = &Objects[Interp_objnum];
-		if (objp->type == OBJ_SHIP)
-			Interp_base_frametime = Ships[objp->instance].base_texture_anim_frametime;
-	}
-
 	int is_invisible = 0;
 
 	if (Interp_warp_bitmap < 0) {
@@ -1958,7 +1949,7 @@
 	dc_printf("model_darkening set to %.1f\n", Interp_depth_scale);
 }
 
-void model_render(int model_num, matrix *orient, vec3d * pos, uint flags, int objnum, int lighting_skip, int *replacement_textures)
+void model_render(int model_num, matrix *orient, vec3d * pos, uint flags, int objnum, int lighting_skip, int *replacement_textures, const bool is_skybox)
 {
 	int cull = 0;
 	// replacement textures - Goober5000
@@ -1997,6 +1988,20 @@
 		cull = gr_set_cull(0);
 	}
 
+	// Goober5000
+	Interp_base_frametime = 0;
+
+	if (objnum >= 0) {
+		object *objp = &Objects[objnum];
+
+		if (objp->type == OBJ_SHIP) {
+			Interp_base_frametime = Ships[objp->instance].base_texture_anim_frametime;
+		}
+	} else if (is_skybox) {
+		Interp_base_frametime = Skybox_timestamp;
+	}
+
+
 	Interp_objnum = objnum;
 
 	if ( flags & MR_NO_LIGHTING )	{
@@ -2980,13 +2985,6 @@
 		cull = gr_set_cull(1);
 	}
 
-	// Goober5000
-	Interp_base_frametime = 0;
-
-	if ( (objp != NULL) && (objp->type == OBJ_SHIP) ) {
-		Interp_base_frametime = Ships[objp->instance].base_texture_anim_frametime;
-	}
-
 	if ( !(Interp_flags & MR_NO_LIGHTING) ) {
 		gr_set_lighting(true, true);
 	}
Index: code/parse/sexp.cpp
===================================================================
--- code/parse/sexp.cpp	(revision 10918)
+++ code/parse/sexp.cpp	(working copy)
@@ -639,7 +639,7 @@
 	{ "nebula-change-storm",			OP_NEBULA_CHANGE_STORM,					1,	1,			SEXP_ACTION_OPERATOR,	},	// phreak
 	{ "nebula-toggle-poof",				OP_NEBULA_TOGGLE_POOF,					2,	2,			SEXP_ACTION_OPERATOR,	},	// phreak
 	{ "nebula-change-pattern",			OP_NEBULA_CHANGE_PATTERN,				1,	1,			SEXP_ACTION_OPERATOR,	},	// Axem
-	{ "set-skybox-model",				OP_SET_SKYBOX_MODEL,					1,	7,			SEXP_ACTION_OPERATOR,	},	// taylor
+	{ "set-skybox-model",				OP_SET_SKYBOX_MODEL,					1,	8,			SEXP_ACTION_OPERATOR,	},	// taylor
 	{ "set-skybox-orientation",			OP_SET_SKYBOX_ORIENT,					3,	3,			SEXP_ACTION_OPERATOR,	},	// Goober5000
 	{ "set-ambient-light",				OP_SET_AMBIENT_LIGHT,					3,	3,			SEXP_ACTION_OPERATOR,	},	// Karajorma
 
@@ -16611,8 +16611,15 @@
 	strcpy_s(new_skybox_model, CTEXT(n));
 	int new_skybox_model_flags = DEFAULT_NMODEL_FLAGS;
 
+	// check if we need to reset the animated texture timestamp
+	n = CDR(n);
+	if (n == -1 || !is_sexp_true(n)) {
+		Skybox_timestamp = game_get_overall_frametime();
+	}
+
+	if (n != -1) n = CDR(n);
+
 	// gather any flags
-	n = CDR(n);
 	while (n != -1) {
 		// this should check all entries in Skybox_flags
 		if ( !stricmp("add-lighting", CTEXT(n) )) {
@@ -27038,7 +27045,9 @@
 		case OP_SET_SKYBOX_MODEL:
 			if (argnum == 0)
 				return OPF_SKYBOX_MODEL_NAME;
-			else if (argnum <= 7)
+			else if (argnum == 1)
+				return OPF_BOOL;
+			else
 				return OPF_SKYBOX_FLAGS;
 
 		case OP_SET_SKYBOX_ORIENT:
@@ -32626,7 +32635,8 @@
 	{ OP_SET_SKYBOX_MODEL, "set-skybox-model\r\n"
 		"\tSets the current skybox model.  Takes 1-7 arguments\r\n"
 		"\t1:\tModel filename (with .pof extension) to switch to\r\n"
-		"\t2-7:\tSet or unset the following skyboxes flags\r\n"
+		"\t2:\tKeep skybox animated texture timestamp (optional, defaults to false)\r\n"
+		"\t3-8:\tSet or unset the following skyboxes flags\r\n"
 		"\t\t\tadd-lighting, no-transparency, add-zbuffer\r\n"
 		"\t\t\tadd-culling, no-glowmaps, force-clamp\r\n\r\n"
 		"Note: If the model filename is set to \"default\" with no extension then it will switch to the mission supplied default skybox."
Index: code/starfield/starfield.cpp
===================================================================
--- code/starfield/starfield.cpp	(revision 10918)
+++ code/starfield/starfield.cpp	(working copy)
@@ -2152,7 +2152,7 @@
 	// draw the model at the player's eye with no z-buffering
 	model_set_alpha(1.0f);
 
-	model_render(Nmodel_num, &Nmodel_orient, &Eye_position, Nmodel_flags);
+	model_render(Nmodel_num, &Nmodel_orient, &Eye_position, Nmodel_flags, -1, -1, NULL, true);
 
 	if (Nmodel_bitmap >= 0) {
 		model_set_forced_texture(-1);
animated textures 3.patch (6,973 bytes)   

MageKing17

2014-07-15 20:53

developer   ~0016075

Second patch reuploaded to correct a slight problem with FRED; new, third patch uploaded that avoids the MR_SKYBOX flag altogether.

Goober5000

2014-07-16 02:36

administrator   ~0016076

Last edited: 2014-07-16 02:36

I would expect that FREDders will want the textures to begin animating when a skybox is activated (such as when you change to it from a previous skybox). For this, you'd need to maintain a base_frametime variable and update it in the sexp that changes the model. You'd then need to reference this variable when you check for MR_SKYBOX.

EDIT: I see from your patch that you've already considered this. Cool.

The_E

2014-07-20 18:03

administrator   ~0016100

Fix committed to trunk@10927.

Goober5000

2014-07-20 20:16

administrator   ~0016102

Reopening because there is one last thing that I didn't notice until the patch was committed. By inserting a sexp argument in the *middle* of an existing sexp, you'll need to handle the case where an old mission specifies arguments in the old order. It looks like your code doesn't account for that possibility. You'll need to check this in both the sexp execution and in the mission parsing. (This is why most additions to sexp arguments put the new argument at the end.)

MageKing17

2014-07-20 20:45

developer   ~0016103

This wasn't done for two reasons: one, because trying to add it at the end would be exceedingly difficult (due to the way those flags are handled), and two, because those other arguments were also added in nightlies and therefore have never been present in a stable release of FSO (not counting release candidates).

The number of missions affected is so small that I personally volunteer to fix them for anybody who doesn't want to add a ( false ) node by hand.

Goober5000

2014-07-21 00:25

administrator   ~0016107

Oh, you're correct -- those flags were only added on June 5th. That's fine then.

A few last points:
1) Do we even need the boolean argument? Is there any situation where we would switch a skybox model and *not* start the animation from the beginning?
2) Since when you add an optional boolean argument in FRED it adds "true", I would recommend flipping around the argument so that "true" is what we want most of the time. So I recommend that "true" resets the timestamp and "false" doesn't.
3) Your average user of FRED is not going to know the significance of resetting the timestamp. I would recommend changing the sexp help to say something like "restarts the skybox texture animation".

MageKing17

2014-07-21 00:43

developer   ~0016108

1) It was trivial for me to imagine a modder wanting to randomly switch between multiple animated skyboxes without each switch restarting the animation from the beginning (and Axem indicated an interest in such a feature after the fact).
2&3) These both make perfect sense; I basically just pulled the description and default value out of a hat. If somebody wants to change these, feel free... I won't have time to write a patch for at least three hours.

MageKing17

2014-07-21 04:54

developer  

sexp.cpp.patch (936 bytes)   
Index: code/parse/sexp.cpp
===================================================================
--- code/parse/sexp.cpp	(revision 10928)
+++ code/parse/sexp.cpp	(working copy)
@@ -16613,7 +16613,7 @@
 
 	// check if we need to reset the animated texture timestamp
 	n = CDR(n);
-	if (n == -1 || !is_sexp_true(n)) {
+	if (n == -1 || is_sexp_true(n)) {
 		Skybox_timestamp = game_get_overall_frametime();
 	}
 
@@ -32635,7 +32635,7 @@
 	{ OP_SET_SKYBOX_MODEL, "set-skybox-model\r\n"
 		"\tSets the current skybox model.  Takes 1-7 arguments\r\n"
 		"\t1:\tModel filename (with .pof extension) to switch to\r\n"
-		"\t2:\tKeep skybox animated texture timestamp (optional, defaults to false)\r\n"
+		"\t2:\tRestart animated textures (optional, defaults to true)\r\n"
 		"\t3-8:\tSet or unset the following skyboxes flags\r\n"
 		"\t\t\tadd-lighting, no-transparency, add-zbuffer\r\n"
 		"\t\t\tadd-culling, no-glowmaps, force-clamp\r\n\r\n"
sexp.cpp.patch (936 bytes)   

MageKing17

2014-07-21 04:55

developer   ~0016111

Deleted the old, irrelevant patches and attached a patch to change the default to "true" and change the description.

Goober5000

2014-07-22 01:09

administrator   ~0016115

Good stuff. Committed.

Related Changesets

fs2open: trunk r10927

2014-07-20 14:27

The_E


Ported: N/A

Details Diff
Fix for Mantis 3076 from MageKing17: Fixes behaviour of animated textures on skyboxes Affected Issues
0003076
mod - /trunk/fs2_open/code/parse/sexp.cpp Diff File
mod - /trunk/fs2_open/code/starfield/starfield.cpp Diff File
mod - /trunk/fs2_open/code/freespace2/freespace.cpp Diff File
mod - /trunk/fs2_open/code/globalincs/systemvars.cpp Diff File
mod - /trunk/fs2_open/code/freespace2/freespace.h Diff File
mod - /trunk/fs2_open/code/globalincs/systemvars.h Diff File
mod - /trunk/fs2_open/code/model/modelinterp.cpp Diff File
mod - /trunk/fs2_open/code/model/model.h Diff File

fs2open: trunk r10931

2014-07-21 21:33

Goober5000


Ported: N/A

Details Diff
MageKing17's tweaks, with a couple of my own Affected Issues
0003076
mod - /trunk/fs2_open/code/parse/sexp.cpp Diff File

Issue History

Date Modified Username Field Change
2014-07-11 06:24 MageKing17 New Issue
2014-07-11 06:24 MageKing17 Status new => assigned
2014-07-11 06:24 MageKing17 Assigned To => MageKing17
2014-07-11 06:24 MageKing17 File Added: animated textures.patch
2014-07-11 06:25 MageKing17 Status assigned => feedback
2014-07-12 16:02 MageKing17 File Added: animated textures 2.patch
2014-07-12 16:04 MageKing17 Note Added: 0016068
2014-07-12 16:04 MageKing17 Status feedback => assigned
2014-07-12 16:04 MageKing17 Status assigned => code review
2014-07-12 16:51 MageKing17 Additional Information Updated
2014-07-15 10:37 m_m Note Added: 0016072
2014-07-15 12:32 The_E Note Added: 0016073
2014-07-15 15:34 MageKing17 Note Added: 0016074
2014-07-15 20:52 MageKing17 File Deleted: animated textures 2.patch
2014-07-15 20:52 MageKing17 File Added: animated textures 2.patch
2014-07-15 20:52 MageKing17 File Added: animated textures 3.patch
2014-07-15 20:53 MageKing17 Note Added: 0016075
2014-07-16 02:36 Goober5000 Note Added: 0016076
2014-07-16 02:36 Goober5000 Note Edited: 0016076
2014-07-20 18:03 The_E Changeset attached => fs2open trunk r10927
2014-07-20 18:03 The_E Note Added: 0016100
2014-07-20 18:03 The_E Status code review => resolved
2014-07-20 18:03 The_E Resolution open => fixed
2014-07-20 20:16 Goober5000 Note Added: 0016102
2014-07-20 20:16 Goober5000 Status resolved => assigned
2014-07-20 20:16 Goober5000 Resolution fixed => reopened
2014-07-20 20:45 MageKing17 Note Added: 0016103
2014-07-21 00:25 Goober5000 Note Added: 0016107
2014-07-21 00:43 MageKing17 Note Added: 0016108
2014-07-21 04:53 MageKing17 File Deleted: animated textures 2.patch
2014-07-21 04:53 MageKing17 File Deleted: animated textures.patch
2014-07-21 04:54 MageKing17 File Added: sexp.cpp.patch
2014-07-21 04:55 MageKing17 Note Added: 0016111
2014-07-21 04:59 MageKing17 Status assigned => code review
2014-07-21 04:59 MageKing17 Resolution reopened => open
2014-07-22 01:09 Goober5000 Note Added: 0016115
2014-07-22 01:09 Goober5000 Status code review => resolved
2014-07-22 01:09 Goober5000 Resolution open => fixed
2014-07-22 01:09 Goober5000 Fixed in Version => 3.7.2
2014-07-22 01:09 Goober5000 Changeset attached => fs2open trunk r10931