Animations

[instafluff0]

PR three of three for R28. This PR is less dramatic in terms of changes than it seems, as most of the 7.8k line additions are INI, config, and Python files for processing animations outside of the game. About 2.5k lines are in injected_code.c (and much of that is config parsing).

The general approach is to piggyback on Civ3’s existing tile-effect machinery instead of creating a new animation workflow from scratch. When a tile matches a configured ambient animation rule, we use Tile_spawn_animated_effect to spawn a normal built-in tile effect (AE_Disorder) as a carrier. So we use a vanilla animated effect path that already knows how to allocate, position, update, and draw an effect. While AE_Disorder is being loaded, I set a temporary override so Units_Image_Data_load_animated_effect ignores the AE_Disorder’s normal art and instead loads the configured custom Art\Animations...*.INI. After vanilla finishes constructing the effect, we rewrite the effect id back to the custom id and apply per-animation direction and pixel offsets. So just as with smolder & disorder, the native animation system still owns lifetime, frame advancement, AMB/FLC handling, and rendering; we just swap in an INI and decide which tiles should spawn or clear those effects.

Here are the main features:

1. Allows custom FLC tile animations to be configured for terrain, resources, PCX sprite indices, and coastal waves, using:

enable_custom_animations = true

---

2. Adds a new tile animation config file with #Animation entries, for example:

#Animation
name = Fish
ini_path = Resources\Fish\Fish.INI
type = resource
resource_type = Fish

---

3. Allows animations to be filtered by day/night hour and season:

show_in_day_night_hours = 7-17
show_in_seasons = winter

---

4. Supports animation placement controls, including direction, x/y offsets, and playback speed:

direction = southeast
x_offset = -15
y_offset = -20
frame_time_seconds = 0.13

---

5. Adds animation support for natural wonders through their config entries:

animation = ini=NaturalWonders\AngelFalls.INI; hours=7-17; seasons=spring,summer,fall,winter; offsets=0,-10

Note that natural wonders can have multiple animation entries.

---

6. Adds tie-breaking logic (pick_tile_animation_winner_for_tile and get_tile_animation_type_priority) for tiles with multiple matching candidate animations, using this priority order:

Resource > Natural Wonder > PCX > Terrain > Coastal Wave

If still tied, animations with day/night or season filters take precedence, then the later config entry wins.

[instafluff0]

Ok, just to follow up on our earlier conversation, this branch should be good to review now. I've merged in changes from master and cleaned up line endings, etc. I've also tested extensively on multiple games as well.

One thing I'm not sure about is the type = pcx animations, which is really just pairing an animation to a given indexed sprite chunk from a terrain PCX, like "index 5 from snow mountains". The code for all of that is really fairly unique and works a bit differently from other animations. The main use case I was thinking about that was animating rivers - which works - but in practice I'm not sure anyone will ever really use it, and it's a good amount of code. So we can consider dropping that if you want.

Also, I put "r28" and "r29" in these branch names to help me track them, and don't mean to presume they should necessarily be in any version. I know there have been a ton of other great changes lately so if you want to put these on a different schedule and release the current code sooner, etc. that's totally fine.

[maxpetul]

I'm about halfway done reading through the code and so far it looks pretty good. I have a few small nitpicks but there's nothing really wrong with it. However I have encountered some problems when I tried the animations in game. They appear and play properly but there are two issues:
First, I get a config error when I try to load a save with custom animations turned on:

Second, when I get in game, no non-animated resources appear. So e.g. I can see whales which are playing their animations but wheat is invisible. Even turning custom animations off doesn't fix that, it just means that no resources are drawn at all.

[instafluff0]

I'm about halfway done reading through the code and so far it looks pretty good. I have a few small nitpicks but there's nothing really wrong with it.

Awesome! Thank you very much for reviewing. And please let me know nitpicks when you're ready; I'm happy to update.

---

First, I get a config error when I try to load a save with custom animations turned on:

15c24c0. - Yeah this was wacky. I have only the vaguest memory of, at some point, realizing that there is - commonly but not always - an "e" in the plural of volcano and added that in as allowed by the parser. Why that was only in the instafluff-current branch I oddly have no recollection. Anyway, adding that in seems to resolve things.

---

Second, when I get in game, no non-animated resources appear. So e.g. I can see whales which are playing their animations but wheat is invisible. Even turning custom animations off doesn't fix that, it just means that no resources are drawn at all.

f7d2058 - Thanks for pointing this out. I found this extremely strange, as I never intended custom animations to tie in or rely on districts, but inadvertently they did. Essentially, the custom resource animation hook could suppress the normal static resource draw, but only reliably fell back to the original resource renderer inside the districts/natural wonders code path. I had only been testing in my scenario with districts on, so I never noticed this. Please give it another go and let me know if this doesn't resolve the issue.

[maxpetul]

Please give it another go and let me know if this doesn't resolve the issue.

Alright, that's fixed now.

---

I've finished reading the code and have found a couple more issues:

1. Regarding those loops in patch_Unit_move_to_adjacent_tile that set redraw_after_move: This feels like a hacky fix, is it really necessary? Do you have a test scenario or something where the issue that comment mentions ("a newly revealed tile shows both the static resource overlay and the animated one") reliably appears? I tried a simple case but didn't see the issue. I'm kind of hoping this bit of logic is not necessary because doing it properly would require some work. 21 is not the correct number of tiles to loop over for the area visible around a unit. Land units can see up to a 5x5 area around them in the standard game, and sea units can see up to 7x7 if they're fortified. Even worse, I've recently merged a PR, Allow configuration of unit view distance #41, that greatly expands and complexifies the unit visibility rules so it's hard to know now what tiles are potentially visible for any given unit. Would this sort of thing be better done in Leader::reveal_tile?
2. While discussing this PR with Codex, it said there's a potential issue with stale animations not being cleared. It pointed to the fact that tile_animation_scheduler_tick skips over tiles with animations already playing (if (tile->Body.active_tile_effect != NULL) continue;) and noted that because of that the animation playing on a tile can't change when the winning animation changes. That's potentially a problem if, for example, there's an animated effect tied to forest terrain playing on a tile when the forest gets cleared. The forest effect wouldn't disappear until the next turn (if I and Codex are reading the code correctly; I haven't tested that).
   There's another related issue in that if a tile has a resource animation playing and the resource is exhausted or otherwise removed from the tile then the animation continues playing indefinitely. Codex says: "Resource drawing only calls clear_active_custom_tile_animation_if_different when it finds a new matching animation, while clear_stale_custom_tile_animation_effects skips resource effects." I've verified in game that resource animations persist after the underlying resource has been exhausted.

I'm not sure how much work these issues deserve so I'll leave it up to you to decide. I haven't confirmed any bugs around the first one, and for the second I've only confirmed that resource anims persist for exhausted resources. If you don't think it's worth the hassle to update terrain animations as soon as a forest is cleared, that's fine with me.

Now for the nitpicks:

• patch_view_all_tile_animations_in_debug_mode in the c3x_config struct is not used.
• The read_tile_animation_direction_value method is pointless, it just wraps another.
• In clear_stale_custom_tile_animation_effects and elsewhere, checking if p_main_screen_form is NULL is really not necessary. It's a preprocessor macro not a variable, so it would take effort to make it NULL. That would never happen accidentally.
• In clear_tile_animation_pcx_sprite_lookup and clear_tile_animation_pcx_rule_lookup, it's not necessary to zero the table struct because table_deinit already does.
• In tile_animation_rule_matches_tile, there's a junk line (void)for_draw;. Also, that whole if block does nothing if for_draw is false, that should be checked at the beginning not the end.
• It's a bit inconsistent how read_pinned_season_for_seasonal_cycle accepts "autumn" as a synonym for "fall" but parse_tile_animation_season_list doesn't. Similarly, the former accepts capitalized season names but the latter doesn't.

[instafluff0]

Thanks for reviewing, I'll follow up on all of these. Just a quick note on (2) though: the intent of this PR was non-temporal animations, essentially those we don't have to worry about clearing during the turn, only interturn. The intent is for the latter to be handled by additions in the next PR, the temporal animations one. That's a great point about exhausted resources though, I hadn't realized that.

[maxpetul]

Alright. I was wondering what "temporal animations" meant but obviously haven't looked into that PR yet.

[instafluff0]

Regarding those loops in patch_Unit_move_to_adjacent_tile that set redraw_after_move: This feels like a hacky fix, is it really necessary? Do you have a test scenario or something where the issue that comment mentions ("a newly revealed tile shows both the static resource overlay and the animated one") reliably appears?

Sure, please see attached zip/save here, and gifs below. I wasn't able to recreate the "showing both static PCX and animation at the same time" bug that I'd run into the past (not sure what I had changed there, but that was earlier in dev), but in any case, we'd still have a timing/sync issue.
Lincoln of the Americans, 3950 BC.SAV.zip

Without tile reveal check:

With tile reveal check:

But I take your point and liked your idea around using Leader::reveal_tile. Let me know what you think about b256003. This is much cleaner, and I found that simply flipping *(bool *)(p_main_screen_form->animator.field_18E4 + 10) = true; should be all we need:

Note that I did end up rolling the logic to check if the tile needed any animation into a new function, tile_has_matching_custom_animation_for_draw, instead of using tile_has_matching_resource_animation_for_draw alone, as in theory the tile could have other animations besides resources that should be immediately queued.

[instafluff0]

• patch_view_all_tile_animations_in_debug_mode in the c3x_config struct is not used.
• The read_tile_animation_direction_value method is pointless, it just wraps another.
• In clear_stale_custom_tile_animation_effects and elsewhere, checking if p_main_screen_form is NULL is really not necessary. It's a preprocessor macro not a variable, so it would take effort to make it NULL. That would never happen accidentally.
• In clear_tile_animation_pcx_sprite_lookup and clear_tile_animation_pcx_rule_lookup, it's not necessary to zero the table struct because table_deinit already does.
• In tile_animation_rule_matches_tile, there's a junk line (void)for_draw;. Also, that whole if block does nothing if for_draw is false, that should be checked at the beginning not the end.
• It's a bit inconsistent how read_pinned_season_for_seasonal_cycle accepts "autumn" as a synonym for "fall" but parse_tile_animation_season_list doesn't. Similarly, the former accepts capitalized season names but the latter doesn't.

Thank you, please see c723d61 and ae84dad

---

There's another related issue in that if a tile has a resource animation playing and the resource is exhausted or otherwise removed from the tile then the animation continues playing indefinitely. Codex says: "Resource drawing only calls clear_active_custom_tile_animation_if_different when it finds a new matching animation, while clear_stale_custom_tile_animation_effects skips resource effects." I've verified in game that resource animations persist after the underlying resource has been exhausted.

Please see 8fd1d58 for this one. I don't think the logic for clearing resource animations should be in clear_stale_custom_tile_animation_effects, as unlike other animation types resources are civ-specific.

I don't have a handy save though where a resource was exhausted, so I haven't been able to test a before and after, and am not sure how best to verify that. Can you do a quick test, if you still have that handy, or let me know what you did so I can reproduce the check?

[maxpetul]

Sure, please see attached zip/save here, and gifs below. I wasn't able to recreate the "showing both static PCX and animation at the same time" bug that I'd run into the past

Alright, I see it now. My earlier test was to look at a fish resource after it's been revealed from the fog by third-ring vision from a fortified galley. I figured that ought to show the bug since the hacky fix in move_to_adjacent_tile wouldn't apply there. But that case is not affected by the bug presumably because the map is completely redrawn between turns. When that test failed to show the bug, I just assumed it was something subtle.

But I take your point and liked your idea around using Leader::reveal_tile. Let me know what you think about b256003.

Looks good to me. I verified just in case that this also works for units with extra visibility beyond what's possible in the base game, which the original fix wouldn't have.

Thank you, please see c723d61 and ae84dad

This all looks good too.

I don't have a handy save though where a resource was exhausted, so I haven't been able to test a before and after, and am not sure how best to verify that. Can you do a quick test, if you still have that handy, or let me know what you did so I can reproduce the check?

Sure, I reran my previous test and found that this bug is now fixed. My test setup is quite simple actually. I created a small scenario with only one visible, animated, connected resource on the map. I chose ivory and made it a strategic resource so it could be exhausted. Then in a debugger I put a breakpoint at 0x4F4E44 which is the instruction after the call to rand_int for resource exhaustion. When the breakpoint hit, I overwrote its return value in EAX to zero to guarantee that the resource would exhaust that turn.

[instafluff0]

Got it, sounds great. Any other changes you want me to make, or looks ok?

[maxpetul]

None, this all looks good to me. Thanks for the contribution.

I'm thinking of posting R28 Preview 1 soon-ish. It's a good time as this was the last PR that was originally tagged as part of R28, as I recall, and it's been quite a while since the last release. I'll make it a preview version since a lot has been added like the combat counter system and configurable visibility, and there could still be bugs. There's just one more change I want to make that I've been putting off.