Source Code Project Mantis - FSSCP
View Issue Details
0003076FSSCPgraphicspublic2014-07-11 02:242014-07-21 21:09
ReporterMageKing17 
Assigned ToMageKing17 
PrioritylowSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version 
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.
Attached Filespatch animated textures 3.patch (6,973) 2014-07-15 16:52
http://scp.indiegames.us/mantis/file_download.php?file_id=2473&type=bug
patch sexp.cpp.patch (936) 2014-07-21 00:54
http://scp.indiegames.us/mantis/file_download.php?file_id=2485&type=bug

Notes
(0016068)
MageKing17   
2014-07-12 12:04   
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.
(0016072)
m_m   
2014-07-15 06:37   
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.
(0016073)
The_E   
2014-07-15 08:32   
Why did you add MR_SKYBOX to the default flags?
(0016074)
MageKing17   
2014-07-15 11:34   
DEFAULT_NMODEL_FLAGS are used for the Skybox only. Specifically, the skybox model number is the global "Nmodel_num".
(0016075)
MageKing17   
2014-07-15 16:53   
Second patch reuploaded to correct a slight problem with FRED; new, third patch uploaded that avoids the MR_SKYBOX flag altogether.
(0016076)
Goober5000   
2014-07-15 22: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.

(0016100)
The_E   
2014-07-20 14:03   
Fix committed to trunk@10927.
(0016102)
Goober5000   
2014-07-20 16:16   
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.)
(0016103)
MageKing17   
2014-07-20 16:45   
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.
(0016107)
Goober5000   
2014-07-20 20:25   
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".
(0016108)
MageKing17   
2014-07-20 20:43   
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.
(0016111)
MageKing17   
2014-07-21 00:55   
Deleted the old, irrelevant patches and attached a patch to change the default to "true" and change the description.
(0016115)
Goober5000   
2014-07-21 21:09   
Good stuff. Committed.

Issue History
2014-07-11 02:24MageKing17New Issue
2014-07-11 02:24MageKing17Statusnew => assigned
2014-07-11 02:24MageKing17Assigned To => MageKing17
2014-07-11 02:24MageKing17File Added: animated textures.patch
2014-07-11 02:25MageKing17Statusassigned => feedback
2014-07-12 12:02MageKing17File Added: animated textures 2.patch
2014-07-12 12:04MageKing17Note Added: 0016068
2014-07-12 12:04MageKing17Statusfeedback => assigned
2014-07-12 12:04MageKing17Statusassigned => code review
2014-07-12 12:51MageKing17Additional Information Updatedbug_revision_view_page.php?rev_id=868#r868
2014-07-15 06:37m_mNote Added: 0016072
2014-07-15 08:32The_ENote Added: 0016073
2014-07-15 11:34MageKing17Note Added: 0016074
2014-07-15 16:52MageKing17File Deleted: animated textures 2.patch
2014-07-15 16:52MageKing17File Added: animated textures 2.patch
2014-07-15 16:52MageKing17File Added: animated textures 3.patch
2014-07-15 16:53MageKing17Note Added: 0016075
2014-07-15 22:36Goober5000Note Added: 0016076
2014-07-15 22:36Goober5000Note Edited: 0016076bug_revision_view_page.php?bugnote_id=16076#r872
2014-07-20 14:03The_EChangeset attached => fs2open trunk r10927
2014-07-20 14:03The_ENote Added: 0016100
2014-07-20 14:03The_EStatuscode review => resolved
2014-07-20 14:03The_EResolutionopen => fixed
2014-07-20 16:16Goober5000Note Added: 0016102
2014-07-20 16:16Goober5000Statusresolved => assigned
2014-07-20 16:16Goober5000Resolutionfixed => reopened
2014-07-20 16:45MageKing17Note Added: 0016103
2014-07-20 20:25Goober5000Note Added: 0016107
2014-07-20 20:43MageKing17Note Added: 0016108
2014-07-21 00:53MageKing17File Deleted: animated textures 2.patch
2014-07-21 00:53MageKing17File Deleted: animated textures.patch
2014-07-21 00:54MageKing17File Added: sexp.cpp.patch
2014-07-21 00:55MageKing17Note Added: 0016111
2014-07-21 00:59MageKing17Statusassigned => code review
2014-07-21 00:59MageKing17Resolutionreopened => open
2014-07-21 21:09Goober5000Note Added: 0016115
2014-07-21 21:09Goober5000Statuscode review => resolved
2014-07-21 21:09Goober5000Resolutionopen => fixed
2014-07-21 21:09Goober5000Fixed in Version => 3.7.2
2014-07-21 21:09Goober5000Changeset attached => fs2open trunk r10931