View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0003076 | FSSCP | graphics | public | 2014-07-11 06:24 | 2014-07-22 01:09 |
Reporter | MageKing17 | Assigned To | MageKing17 | ||
Priority | low | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Target Version | 3.7.2 | Fixed in Version | 3.7.2 | ||
Summary | 0003076: Animated textures in a skybox behave counter-intuitively | ||||
Description | model_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 Reproduce | Axem has helpfully created a test mod that demonstrates the issue: http://www.mediafire.com/download/qsi2phgt5982be0/SillySkybox.rar | ||||
Tags | No tags attached. | ||||
|
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. |
|
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. |
|
Why did you add MR_SKYBOX to the default flags? |
|
DEFAULT_NMODEL_FLAGS are used for the Skybox only. Specifically, the skybox model number is the global "Nmodel_num". |
|
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); |
|
Second patch reuploaded to correct a slight problem with FRED; new, third patch uploaded that avoids the MR_SKYBOX flag altogether. |
|
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. |
|
Fix committed to trunk@10927. |
|
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.) |
|
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. |
|
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". |
|
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. |
|
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" |
|
Deleted the old, irrelevant patches and attached a patch to change the default to "true" and change the description. |
|
Good stuff. Committed. |
fs2open: trunk r10927 2014-07-20 14:27 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 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 |
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 |