Add basic bombard by land units by ajhalme · Pull Request #928 · C7-Game/OpenCiv3

ajhalme · 2026-04-29T00:57:25Z

Basic bombard mechanic based on #927.

Changes

base-ruleset.json extension with bombardRange and rateofFire for all units
unit_bombard unit button enabled, including "B" shortcut key
Texture based crosshair & disallowed cursor (Godot native)
Mouse-based interactive target selection
BombardLayer to render bombard grid, cursor, target selection
BombardInfo, in the spirit of GotoInfo, to carry information about user inputs to BombardLayer
Fixed/re-enabled "effect" animations for the bombard explosion
Extend bombard(..) MapUnit logic with rate of fire (and hit notifications)
Bombard target tile collection enumerator in Tile, GetTilesWithinBombardRange(..) -- Rank-based enumerator ignores the "corners" that are needed for the bombard grid. (Think "Rank-2 City BFC" vs "complete 5x5 tile square".)

Extras

If a saved game is set up such that an opponent civ is about to attack, CreateGame will get stuck in an infinite loop, waiting for animations. This PR includes a workaround that disabled animations on CreateGame load.

Demo

…file parsing, bombard range

ajhalme · 2026-04-29T00:59:40Z

Known issues (Update: Resolved!):

Bombard before declaring war
Bombing your own units
Lethal bombard

stavrosfa

Good stuff, thanks!

I didn't playtest this yet, but I will and let you know if I have any more thoughts

stavrosfa · 2026-04-29T16:04:12Z

+
+		public override async void process() {
+			MapUnit unit = EngineStorage.gameData.GetUnit(unitID);
+			Tile tile = EngineStorage.gameData.map.tileAt(tileX, tileY);


What is the purpose of passing the Tile in the constructor, extracting the X,Y coords, and the get the Tile itself from the coords? Why not use the Tile/TileId directly?

Mainly just trying to follow the existing message patterns here.

This is the UI--Engine boundary, so having the message be easy to serialise is probably a good idea. If the message can be fully described with primitive values and then hydrated to the actual game object when processed, that's probably a win for portability, multiplayer, etc. Tile Id could work here as well, I haven't really looked at the usage patterns, but coordinates at least should be fairly stable in any game.

Sorry, which pattern are you referring to here? I am not seeing anything similar in the other messages.

Messages that can be serialized, is something that I haven't though of directly for this game.

The pattern so far seems to be to use DTOs instead of directly serializing an object, so I would guess that's how it would be for messages too, as they handle City, TilePath or MapUnit objects at the moment directly.

Anyway, I don't have a problem with this, if it were me, I would simplify it as I mentioned, but I don't mind this.

Many of the engine messages in this file (MsgSetUnitPath, MsgMoveUnit, MsgSetFortification, ...) feature a constructor that takes a UnitId from a MapUnit. This id is then hydrated into the MapUnit inside process(..). Why do these messages bother with an id instead of the full MapUnit?

This is the pattern I'm building on:

UnitId --> MapUnit TileId/Coords --> Tile

Any thoughts on engine message data fields vs full DTOs, @esbudylin ?

stavrosfa · 2026-04-29T16:23:20Z

 		this.folderPath = "Art/Animations/" + effectCategories[effect];
 		this.iniFileName = effectINIFileNames[effect];
 		this.action = MapUnit.AnimatedAction.DEATH;
+		this.effect = effect;


Not a comment on this particular PR, but since you had some friction with it now, does this whole system look ok to you (AnimationManager, C7Animation, AnimationTracker, etc)?
Let me know if you have any comments, I am trying to refactor this on the side, make it more versatile and extendible

Current effect animation feels very glued-on on top of the unit animation scheme. Rethinking/refactoring the animation system as a whole makes sense.

The bomber plane bombardment animation might be a good starting point as it's a complex sequence. If you can support that, then everything else is easy.

I kinda like the approach of having the animation scheduling and progress-dependent rendering be separate parts. It's a bit much initially, but I can see how it can be really flexible. Maybe we need a central scheduling scheme and then a few different renderers. Could have simpler tile-based ones for unit and effect, and then a more complex renderer that can handle moving the render target on screen.

stavrosfa · 2026-04-29T16:28:38Z

@@ -162,13 +162,19 @@ private Vector2 GetFramePosition(MapUnit.Appearance appearance, C7Animation unit
 	}

 	public void drawEffectAnimFrame(LooseView looseView, C7Animation anim, float progress, Vector2 tileCenter) {


Another question rather than critique per se, do you think the effects animations in general should live in the Unit Layer? What if we had a dedicated EffectsLayer?

A separate EffectLayer could work fine. The effects generally are unit-linked so they can in my view live alongside the units, but a separate layer could make it easier to, well, layer. The z-index hackery doesn't scale.

stavrosfa · 2026-04-30T10:51:48Z

 			false => throw new Exception($"The provided save does not contain a human player"),
 		};

+		EngineStorage.animationsEnabled = false;


Sorry, I forgot to adress this in the previous review.

This looks inoccent enough, although I am pretty sure it's gonna have side effects. I need to investigate further.
If you have more detailed clues as to why the game gets stuck in an infinite loop, let me know.

I tracked the issue down to the fight(..) method in MapUnit. The comment "Do combat rounds" (line 355) is at the start of a loop that never resolves with the attached save file. It's not actually looping, I think the animation system just isn't ready to receive the animation render requests for the AI civ's fight animation at game load.

Indeed, my save files load differently in OC3 and Civ3, as the same save file in the former starts with AI civs making their moves, while latter starts with human user's (previous?) turn ended.

Attached zipped save file reproduces the issue, getting stuck on await attacker.animateAsync(ATTACK1); in fight(..) (L358 in my codebase).

on_load_animation_bug.json.zip

I still don't get this fully. Does the saved game begin with the AI attacking? If so, it shouldn't be possible. It shouldn't be possible to save the game while someone else is moving in the first place.

If the game was saved at the very end of the round, where you have to press the button/Enter to finish the round, then loading it up begins the round without waiting for the human to trigger it, it's faulty behaviour.

Regardless, I don't think disabling all animations is a valid workaround, there is clearly something else going on here, and we should address this instead.

Also, as far as I understand, this is not bombard related, right? Any attack triggers this

I've removed this workaround. The load issue is not bombard-specific.

But yes, I observed the game loading with AI civ units making moves, including combat, which triggers this bug. I've been able to replicate this with both normal saves and ones I modified by giving the AI civs more units. (I was actually trying to replicate the city crashing issue you mentioned in issue 907 by having all the civs at war with each other: this mayhem led me to this issue initially.)

Anyway, I've created a new save file that reliably results in an all-black map for me:
on_load_animation_bug_2.json.zip

…eting logic

ajhalme · 2026-05-04T23:18:30Z

Added basic improvements bombardment and even city and population bombardment. Had a stab at introducing city walls logic as well.

Added some basic wiring for requiring war declaration, disallowing friendly fire, disallowing bombarding of in-range empty tiles, etc.

Merged in the recent changes, plus the not yet merged test file change.

Good to go - bombard has everything I wanted for now. Didn't explore how things break with naval vessels and air things, but that's for another PR, I reckon.

stavrosfa · 2026-05-05T18:59:57Z

 			false => throw new Exception($"The provided save does not contain a human player"),
 		};

+		EngineStorage.animationsEnabled = false;


I still don't get this fully. Does the saved game begin with the AI attacking? If so, it shouldn't be possible. It shouldn't be possible to save the game while someone else is moving in the first place.

If the game was saved at the very end of the round, where you have to press the button/Enter to finish the round, then loading it up begins the round without waiting for the human to trigger it, it's faulty behaviour.

Regardless, I don't think disabling all animations is a valid workaround, there is clearly something else going on here, and we should address this instead.

Also, as far as I understand, this is not bombard related, right? Any attack triggers this

stavrosfa · 2026-05-05T19:17:15Z

+
+		public override async void process() {
+			MapUnit unit = EngineStorage.gameData.GetUnit(unitID);
+			Tile tile = EngineStorage.gameData.map.tileAt(tileX, tileY);


Sorry, which pattern are you referring to here? I am not seeing anything similar in the other messages.

Messages that can be serialized, is something that I haven't though of directly for this game.

The pattern so far seems to be to use DTOs instead of directly serializing an object, so I would guess that's how it would be for messages too, as they handle City, TilePath or MapUnit objects at the moment directly.

Anyway, I don't have a problem with this, if it were me, I would simplify it as I mentioned, but I don't mind this.

stavrosfa · 2026-05-05T19:23:54Z

+		DrawSquare(looseView, left, top, right, bottom);
+	}
+
+	private void drawTargetBombardTile(LooseView looseView, Vector2 tileCenter) {


I believe it's the same thin brown outline pattern for the target square as in here:

OpenCiv3/C7/Map/TileAssignmentLayer.cs

Line 122 in b8a7c41

private void DrawOccupiedTileSquare(LooseView looseView, Vector2 tileCenter) {

Yes, the mechanism looks similar - but just different enough that making one work like the other is a bit involved. Something to look into for consolidation/refactoring.

I'm also inclined to break up the monolithic MapView.cs file, bring LooseLayer closer to the usage. Maybe then we can have a layer utilities library where we can put some of these reusable pieces.

ajhalme added 9 commits April 24, 2026 00:48

Init bombard (WIP)

b22a72d

Bombard grid via direct draw, basic UI interactions, enine msg, save …

93eb576

…file parsing, bombard range

unit-bombard Godot keymap

7fc12de

Bombard target tile highlight

d692cec

Add rateOfFire

33ef574

Adjsut unitProtos with bombard details

7f86aeb

Disable animation for CreateGame first turn

c2371b7

effect animation, z-indexing, bombard rate of fire and notice

f00f792

Tiles within bombard range

8b0171f

stavrosfa approved these changes Apr 29, 2026

View reviewed changes

stavrosfa reviewed Apr 30, 2026

View reviewed changes

ajhalme added 5 commits May 1, 2026 01:07

Tile square improvements

c3b375f

Bombard refinement: basic city and terrain bombing, more bombard targ…

84fd602

…eting logic

Merge Development

a088bdb

Test file change

fa310d5

Refine bombard rules

d3c28ea

stavrosfa reviewed May 5, 2026

View reviewed changes

ajhalme added 5 commits May 5, 2026 23:26

Remove load-time animation issue workaround

6f02d3c

RemoveCitizen revisited

a0aba9f

remove null conditional

8b744f6

Clean up Terrain Improvement life cycle

fc333a6

Sprinkled some "TODO: Make Configurable" comments

90fafc9

		@@ -162,13 +162,19 @@ private Vector2 GetFramePosition(MapUnit.Appearance appearance, C7Animation unit
		}

		public void drawEffectAnimFrame(LooseView looseView, C7Animation anim, float progress, Vector2 tileCenter) {

Conversation

ajhalme commented Apr 29, 2026

Changes

Extras

Demo

Uh oh!

ajhalme commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stavrosfa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajhalme commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ajhalme commented Apr 29, 2026 •

edited

Loading

ajhalme commented May 4, 2026 •

edited

Loading