View Issue Details [ Jump to Notes ] [ Related Changesets ] | [ Issue History ] [ Print ] | ||||||||
ID | Project | Category | View Status | Date Submitted | Last Update | ||||
---|---|---|---|---|---|---|---|---|---|
0003076 | FSSCP | graphics | public | 2014-07-11 02:24 | 2014-07-21 21:09 | ||||
Reporter | MageKing17 | ||||||||
Assigned To | MageKing17 | ||||||||
Priority | low | Severity | minor | Reproducibility | always | ||||
Status | resolved | Resolution | fixed | ||||||
Product Version | |||||||||
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. | ||||||||
Attached Files |
|
![]() |
|
MageKing17 (developer) 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. |
m_m (developer) 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. |
The_E (administrator) 2014-07-15 08:32 |
Why did you add MR_SKYBOX to the default flags? |
MageKing17 (developer) 2014-07-15 11:34 |
DEFAULT_NMODEL_FLAGS are used for the Skybox only. Specifically, the skybox model number is the global "Nmodel_num". |
MageKing17 (developer) 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. |
Goober5000 (administrator) 2014-07-15 22:36 Last edited: 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. |
The_E (administrator) 2014-07-20 14:03 |
Fix committed to trunk@10927. |
Goober5000 (administrator) 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.) |
MageKing17 (developer) 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. |
Goober5000 (administrator) 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". |
MageKing17 (developer) 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. |
MageKing17 (developer) 2014-07-21 00:55 |
Deleted the old, irrelevant patches and attached a patch to change the default to "true" and change the description. |
Goober5000 (administrator) 2014-07-21 21:09 |
Good stuff. Committed. |
![]() |
|||
fs2open: trunk r10927
Timestamp: 2014-07-20 14:27:56 Author: The_E Ported: N/A [ Details ] [ Diff ] |
Fix for Mantis 3076 from MageKing17: Fixes behaviour of animated textures on skyboxes | ||
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
Timestamp: 2014-07-21 21:33:28 Author: Goober5000 Ported: N/A [ Details ] [ Diff ] |
MageKing17's tweaks, with a couple of my own | ||
mod - /trunk/fs2_open/code/parse/sexp.cpp | [ Diff ] [ File ] | ||
![]() |
|||
Date Modified | Username | Field | Change |
---|---|---|---|
2014-07-11 02:24 | MageKing17 | New Issue | |
2014-07-11 02:24 | MageKing17 | Status | new => assigned |
2014-07-11 02:24 | MageKing17 | Assigned To | => MageKing17 |
2014-07-11 02:24 | MageKing17 | File Added: animated textures.patch | |
2014-07-11 02:25 | MageKing17 | Status | assigned => feedback |
2014-07-12 12:02 | MageKing17 | File Added: animated textures 2.patch | |
2014-07-12 12:04 | MageKing17 | Note Added: 0016068 | |
2014-07-12 12:04 | MageKing17 | Status | feedback => assigned |
2014-07-12 12:04 | MageKing17 | Status | assigned => code review |
2014-07-12 12:51 | MageKing17 | Additional Information Updated | View Revisions |
2014-07-15 06:37 | m_m | Note Added: 0016072 | |
2014-07-15 08:32 | The_E | Note Added: 0016073 | |
2014-07-15 11:34 | MageKing17 | Note Added: 0016074 | |
2014-07-15 16:52 | MageKing17 | File Deleted: animated textures 2.patch | |
2014-07-15 16:52 | MageKing17 | File Added: animated textures 2.patch | |
2014-07-15 16:52 | MageKing17 | File Added: animated textures 3.patch | |
2014-07-15 16:53 | MageKing17 | Note Added: 0016075 | |
2014-07-15 22:36 | Goober5000 | Note Added: 0016076 | |
2014-07-15 22:36 | Goober5000 | Note Edited: 0016076 | View Revisions |
2014-07-20 14:03 | The_E | Changeset attached | => fs2open trunk r10927 |
2014-07-20 14:03 | The_E | Note Added: 0016100 | |
2014-07-20 14:03 | The_E | Status | code review => resolved |
2014-07-20 14:03 | The_E | Resolution | open => fixed |
2014-07-20 16:16 | Goober5000 | Note Added: 0016102 | |
2014-07-20 16:16 | Goober5000 | Status | resolved => assigned |
2014-07-20 16:16 | Goober5000 | Resolution | fixed => reopened |
2014-07-20 16:45 | MageKing17 | Note Added: 0016103 | |
2014-07-20 20:25 | Goober5000 | Note Added: 0016107 | |
2014-07-20 20:43 | MageKing17 | Note Added: 0016108 | |
2014-07-21 00:53 | MageKing17 | File Deleted: animated textures 2.patch | |
2014-07-21 00:53 | MageKing17 | File Deleted: animated textures.patch | |
2014-07-21 00:54 | MageKing17 | File Added: sexp.cpp.patch | |
2014-07-21 00:55 | MageKing17 | Note Added: 0016111 | |
2014-07-21 00:59 | MageKing17 | Status | assigned => code review |
2014-07-21 00:59 | MageKing17 | Resolution | reopened => open |
2014-07-21 21:09 | Goober5000 | Note Added: 0016115 | |
2014-07-21 21:09 | Goober5000 | Status | code review => resolved |
2014-07-21 21:09 | Goober5000 | Resolution | open => fixed |
2014-07-21 21:09 | Goober5000 | Fixed in Version | => 3.7.2 |
2014-07-21 21:09 | Goober5000 | Changeset attached | => fs2open trunk r10931 |