[godot4] Use Godot TileMap for Rendering #407

pcen · 2023-06-21T00:37:45Z

There is a larger performance overhead for rendering the tilemap compared to how the map is currently rendered, but the frame rate does not decrease nearly as much when more of the map becomes visible

The following features are added in pull requests based on this branch:

#408 adds units to this branch - merged
#412 adds cities - merged
#419 adds grid view - merged
#421 adds fog of war - merged
#422 adds horizontal wrapping - merged

pcen · 2023-07-28T16:25:10Z

I'm going to merge this unless there's any objections (plus it can always be reverted). Refactoring large modules to use Godot 4 features involves big diffs (in this case, across all PRs about 2000 and it's still not done). I can't feasibly break this change into, say, 10 different 200 line PRs that are easily reviewable for two reasons

it takes maybe 800 lines to get just base terrain rendering, so I can't think of smaller incremental changes to get tile maps working
even with the 3 current PRs based on the previous branch, rebasing if something in the base branch (godot4) is difficult

Since it's logistically too difficult (for me at least) to keep adding features or refactoring that depend on these changes to get merged I'm effectively blocked in the mean time. I'm happy to wait for reviews, but if merging stale self-reviewed PRs is acceptable I'd like to do so since I'd love to make more progress with moving to godot 4

QuintillusCFC · 2023-07-31T06:04:22Z

Merging stale self-reviewed PRs is acceptable. There's been some debate of waiting 1 week versus 2 (I'm partial to 1, as it can slow things down too much otherwise).

That said, I do have some concerns about replacing the godot4 branch with this one until some more of the incremental changes are done. I still need to check out 408/412 as they may address my concerns, but right now godot4 is getting kind-of close to parity with the base Development (Godot 3) branch, more so than I'd expected. But the TileMap version isn't entirely there yet.

Some of this is related to units and likely fixed in 408/412, some of it is the whole map being visible (no fog of war yet). I also crashed it while pressing Ctrl+G to turn on the grid, with this error:

Stack overflow.
Repeat 130585 times:
--------------------------------
   at C7.Map.MapView.get_showGrid()
--------------------------------
   at Game.processActions()
   at Game._Process(Double)
   at Godot.Node.InvokeGodotClassMethod(Godot.NativeInterop.godot_string_name ByRef, Godot.NativeInterop.NativeVariantPtrArgs, Godot.NativeInterop.godot_variant ByRef)
   at Godot.CanvasItem.InvokeGodotClassMethod(Godot.NativeInterop.godot_string_name ByRef, Godot.NativeInterop.NativeVariantPtrArgs, Godot.NativeInterop.godot_variant ByRef)
   at Godot.Node2D.InvokeGodotClassMethod(Godot.NativeInterop.godot_string_name ByRef, Godot.NativeInterop.NativeVariantPtrArgs, Godot.NativeInterop.godot_variant ByRef)
   at Game.InvokeGodotClassMethod(Godot.NativeInterop.godot_string_name ByRef, Godot.NativeInterop.NativeVariantPtrArgs, Godot.NativeInterop.godot_variant ByRef)
   at Godot.Bridge.CSharpInstanceBridge.Call(IntPtr, Godot.NativeInterop.godot_string_name*, Godot.NativeInterop.godot_variant**, Int32, Godot.NativeInterop.godot_variant_call_error*, Godot.NativeInterop.godot_variant*)

I checked base godot4 and the grid works as expected there so it's something in the switch to TileMap. Grids are by their nature at the very edge of tiles...

I'm curious about the performance not decreasing as much when more of the map becomes visible. Do you mean revealed in-game, or as you use a larger view (e.g. 4K versus 720p), or both, or something different? It would be interesting to see some stats on the relative performance, both the increased overhead and the improvements, if you have them.

QuintillusCFC · 2023-07-31T06:08:21Z

I should note, it's just replacing godot4 that I have concerns with. Kind of like how we want godot4 to reach 98%+ parity with Development before making it the default, IMO before switching to tilemaps they should be at 98%+ parity with the existing rendering. Most likely, this would mean making pcen/use-tilemaps the base for tilemaps, and merging the other tilemap-based changes into it, until as a whole the tilemaps version is as good or better than the non-tilemaps version.

pcen · 2023-07-31T11:20:09Z

I should note, it's just replacing godot4 that I have concerns with. Kind of like how we want godot4 to reach 98%+ parity with Development before making it the default, IMO before switching to tilemaps they should be at 98%+ parity with the existing rendering. Most likely, this would mean making pcen/use-tilemaps the base for tilemaps, and merging the other tilemap-based changes into it, until as a whole the tilemaps version is as good or better than the non-tilemaps version.

Yeah I guess this is where I'm torn, because as you mentioned, for example, the grid crashing is fixed in 408/412 (not sure which off the top of my head). If I merge all of the TileMap changes into this branch so it's ready to replace the base godot4 branch I'd expect a +/- 2500 line PR. Maybe it's better anyways since each 'incremental' PR is so large, at least someone can play-test the full change.

As for performance, by "more map" I mean that as more tiles become visible (at the same resolution) the performance of godot 3.5 drops more. In my case (a 2k monitor as the display for an m1 macbook air) when enough tiles are visible to fill the entire monitor without any fog of war I get ~60fps on 3.5 and ~200fps on 4.x with TileMap. But it would probably be worth doing a slightly more rigorous comparison

QuintillusCFC · 2023-08-03T20:11:47Z

I should note, it's just replacing godot4 that I have concerns with. Kind of like how we want godot4 to reach 98%+ parity with Development before making it the default, IMO before switching to tilemaps they should be at 98%+ parity with the existing rendering. Most likely, this would mean making pcen/use-tilemaps the base for tilemaps, and merging the other tilemap-based changes into it, until as a whole the tilemaps version is as good or better than the non-tilemaps version.

Yeah I guess this is where I'm torn, because as you mentioned, for example, the grid crashing is fixed in 408/412 (not sure which off the top of my head). If I merge all of the TileMap changes into this branch so it's ready to replace the base godot4 branch I'd expect a +/- 2500 line PR. Maybe it's better anyways since each 'incremental' PR is so large, at least someone can play-test the full change.

As for performance, by "more map" I mean that as more tiles become visible (at the same resolution) the performance of godot 3.5 drops more. In my case (a 2k monitor as the display for an m1 macbook air) when enough tiles are visible to fill the entire monitor without any fog of war I get ~60fps on 3.5 and ~200fps on 4.x with TileMap. But it would probably be worth doing a slightly more rigorous comparison

The practice I've found that works best for large changesets of a branch that won't realistically be ready in one PR due to size, is to create a new branch as a base, and review/test incrementally, using that feature base branch. Then when the feature is all done, the with-all-the-changes base branch only needs a basic regression test; its constituent parts have all been reviewed. Basically that is to make sure everything plays nicely together and that there aren't any files missing from the commits (which in theory should have been caught already anyway, and usually that is the case).

That is quite a significant increase. I'm not sure we really have any FPS benchmarks at this point; most of the performance effort has gone towards AI move calculations. But almost 3.5x as good is a nice boost indeed.

Also good to know it runs on an M1; we didn't have any confirmation of that previously. I just acquired one for iOS development, but haven't set up C7 on it yet, and I don't plan to make it my main machine anyway. That means we now have someone who regularly runs each of the "most likely" platforms; me on Windows, you on Apple Silicon, WildWeazel on Linux, and Jim (inactive) on Intel Mac.

pcen · 2023-08-07T10:59:09Z

I think that definitely makes sense, this branch is essentially the base for tilemaps
#408 adds units to this branch
#412 adds cities to #408
#419 adds grid view
#420 adds fog of war
#422 adds horizontal wrapping

once all of these are merged into this branch (and #412 into #408) map rendering should be on par with the current gd4 map

WildWeazel · 2023-08-10T02:39:11Z

I started reviewing this a while back but obviously it's been growing. What files/commits are native to this PR?

pcen · 2023-08-10T13:31:45Z

I started reviewing this a while back but obviously it's been growing. What files/commits are native to this PR?

The diff in this PR is basically the minimal changes required to render terrain using TileMaps. All the other PRs I outlined in my last comment are based on this branch (or some are based on each-other), but all reintroduce features on top of this diff. Once approved / merged, I'm merging them into this branch, which eventually will be one huge diff to merge into the godot4 branch

WildWeazel · 2023-08-10T15:26:22Z

Right, I'd just like to review everything exactly once and I'm not sure how to get a clean diff without the other branches. Was there a last commit directly to this branch before merges started?

pcen · 2023-08-11T03:14:12Z

I believe commit 8e35041 is the last commit before other branches are merged

WildWeazel · 2023-09-04T18:47:52Z

C7/Map/Civ3TerrainTileset.cs

+namespace C7.Map {
+
+	class TerrainPcx {
+		private static Random prng = new Random();


I think we have a "master" RNG somewhere in the game data so that everything can be reproduced from the same seed. If that's not sensible to import here, let's put it somewhere that is.

We should probably have a settings struct that's easy to initialise with default values instead of injecting a GameData into everything. This RNG is purely cosmetic at the moment as it just determines which terrain texture variant to use

WildWeazel · 2023-09-04T19:00:42Z

C7/Map/Civ3TerrainTileset.cs

+		private int abcIndex(string terrain) {
+			List<int> indices = new List<int>();
+			for (int i = 0; i < abc.Count(); i++) {
+				if (abc[i] == terrain) {
+					indices.Add(i);
+				}
+			}
+			return indices[prng.Next(indices.Count)];
+		}


indices = abc.Where(i => i == terrain).Select(i => i.Index).toList() or something like that
If selecting a random item from a collection will be a common operation, we may want an extension method for it

C7/OldMapView.cs

WildWeazel · 2023-09-04T20:13:23Z

C7/Map/Civ3TerrainTileset.cs

Since this file is specific to Civ3 file formats, I'm thinking it should belong in ConvertCiv3Media

It is specific to civ3, but it can support loading from pcx or png where the tiles are laid out in the same layout. unless we're going to create a new layout we could just genericize this implementation slightly

WildWeazel · 2023-09-04T20:15:06Z

C7/Map/TileSetLoader.cs

Also seems like it belongs in ConvertCiv3Media

same as above, unless we change the layout of tilesets, the implementation of this class is 90% generic and 10% hardcoded for civ3 at the moment - instead of hardcoding paths to civ3 files we can configure this somewhere, but I don't think the tileset loading logic belongs in convert civ3 media

WildWeazel · 2023-09-04T20:26:57Z

C7/Map/MapView.cs

+				string top = "coast";
+				if (y > 0) {
+					top = even ? terrain[x, y - 1] : terrain[(x + 1) % width, y - 1];
+				}
+				string bottom = "coast";
+				if (y < height - 1) {
+					bottom = even ? terrain[x, y + 1] : terrain[(x + 1) % width, y + 1];
+				}


Is it valid for these conditions to be false and the strings left as "coast"? What does that imply?

TL;DR, explanation below: these conditions will always be false for tiles on non-wrapping edges. "coast" has no implication here since it's never visible, and is the cleanest default since it always gives a visually correct terrain tile.

civ terrain tiles represent the corners of 4 game tiles - so at the edges of the map, there is 1/4 of a terrain tile that is "off map". When choosing which terrain tile to use for the intersection of the 3 edge tiles, we need to choose a valid terrain tile combination (terrain tiles can only represent the intersection of 3 different terrain types, never 4). One of the following 2 predicates is always true of the terrain types of the corner of 3 tiles at an edge of the map:
A - one of those three tiles is coast => choosing coast for the 4th "fake, off map" tile is OK
B - two of those tiles have the same terrain type (and it is not coast), and the combination of terrain types will allow for the 4th "fake, off map" tile to be coast

If this were not a tilemap (ie. how the 3.x branch does it), we could instead sample from the corners of 4 different terrain tiles to draw the terrain underneath a game tile. Since we need to pick instead a terrain tile to draw under the joining corner of tiles, coast is a legal option which will give us a valid terrain tile to draw, and the fact that the 4th fake tile corner is hard coded to be coast is inconsequential since it's outside of the playable / visible map.

C7/Map/MapView.cs

WildWeazel

Looking great! Just some minor cleanup

benskywalker · 2023-09-23T14:00:55Z

C7/Game.cs

-	}
-	public void _on_DownButton_pressed() {
-		mapView.cameraLocation += new Vector2(0, 64);
+	private void onSliderZoomChanged(float value) {


On my end it looks like zoom isn't working right now. If we either change the function here to _on_Zoom_value_changed, or if we change line 271 of C7Game.tscn to match this function, it starts working again. Might be a worthwhile fix. I'm not sure what naming convention we want to follow here.

thanks, not sure when this broke. It should be named in camel case, not sure why a lot of these callbacks are _ prefixed, maybe when you create a signal in Godot this is/was the default naming convention is uses to generate the function

WildWeazel · 2023-10-12T22:14:19Z

Are you making any new commits directly to this branch or just merging? I think I've reviewed everything up to the first merge.

add Camera2D test drawing units in word coordinates wip simplify unit sprite test demo - AnimationManager could be simpler but trying to keep diff small enable pixel snapping - removes world seams render roads in tilemap add logic for drawing rails on tilemap - missing separate layer working rails layer simple abstraction for loading atlas sources add resources and clear cells when empty track net lines changed working rivers separate notion of layer and atlas in MapView add mountains get rid of incorrect y offsets (tiles are already centered) offset is necessary for oversized tiles... add volcanos single terrain overlay add full left skirt to terrain layer detect edges of map in world space fix bad bounds for forest tiles and use full resource tileset wip tile set loader so specific assets can be overwritten in the future add marsh to tilemap terrain overlay layer camera can center on tiles buildings on tilemap remove ported code load goodyhut pcx pixel perfect transforms dedupe code enable Y sort for each tilemap layer kinda working animations wip wip working sprite animations render best defender on top of unit stack significantly faster animation loop when only doing visible tiles draw cursor remove ported code

test drawing units in word coordinates wip simplify unit sprite test demo - AnimationManager could be simpler but trying to keep diff small enable pixel snapping - removes world seams render roads in tilemap add logic for drawing rails on tilemap - missing separate layer working rails layer simple abstraction for loading atlas sources add resources and clear cells when empty track net lines changed working rivers separate notion of layer and atlas in MapView add mountains get rid of incorrect y offsets (tiles are already centered) offset is necessary for oversized tiles... add volcanos single terrain overlay add full left skirt to terrain layer detect edges of map in world space fix bad bounds for forest tiles and use full resource tileset wip tile set loader so specific assets can be overwritten in the future add marsh to tilemap terrain overlay layer camera can center on tiles buildings on tilemap remove ported code load goodyhut pcx pixel perfect transforms dedupe code enable Y sort for each tilemap layer kinda working animations wip wip working sprite animations render best defender on top of unit stack significantly faster animation loop when only doing visible tiles draw cursor remove ported code wip render cities city labels text renders but at wrong offsets correct city label offsets only redraw city label scene when necessary redraw city scene if production name changes re-implement insure location is in view using player camera remove GD.Print statements emit city built message from engine interaction

add Camera2D test drawing units in word coordinates wip simplify unit sprite test demo - AnimationManager could be simpler but trying to keep diff small enable pixel snapping - removes world seams render roads in tilemap add logic for drawing rails on tilemap - missing separate layer working rails layer simple abstraction for loading atlas sources add resources and clear cells when empty track net lines changed working rivers separate notion of layer and atlas in MapView add mountains get rid of incorrect y offsets (tiles are already centered) offset is necessary for oversized tiles... add volcanos single terrain overlay add full left skirt to terrain layer detect edges of map in world space fix bad bounds for forest tiles and use full resource tileset wip tile set loader so specific assets can be overwritten in the future add marsh to tilemap terrain overlay layer camera can center on tiles buildings on tilemap remove ported code load goodyhut pcx pixel perfect transforms dedupe code enable Y sort for each tilemap layer kinda working animations wip wip working sprite animations render best defender on top of unit stack significantly faster animation loop when only doing visible tiles draw cursor remove ported code

…n wrap function

…ansform value

…g format

WildWeazel · 2024-12-31T04:46:15Z

This was not merged into godot4 for Carthage because it seems a bit more experimental, there are some unresolved questions and regressions called out above, and it had produced some conflicts with the last-minute fixes in godot4. This along with the savegame branch should be prioritized next before they get any further out of date.

pcen changed the title ~~Pcen/use tilemaps~~ Use Godot TileMap for Rendering Jun 21, 2023

pcen mentioned this pull request Jun 25, 2023

Tall terrain graphics don't display greatly #347

Open

pcen marked this pull request as ready for review June 29, 2023 15:22

pcen requested a review from WildWeazel July 3, 2023 20:46

pcen force-pushed the pcen/use-tilemaps branch from fcded29 to 4b89776 Compare July 9, 2023 17:58

pcen force-pushed the pcen/use-tilemaps branch from 4b89776 to 8e35041 Compare July 31, 2023 16:53

WildWeazel reviewed Sep 4, 2023

View reviewed changes

C7/OldMapView.cs Outdated Show resolved Hide resolved

WildWeazel reviewed Sep 4, 2023

View reviewed changes

C7/Map/MapView.cs Show resolved Hide resolved

WildWeazel requested changes Sep 4, 2023

View reviewed changes

pcen force-pushed the pcen/use-tilemaps branch from 34e78b2 to 289eae6 Compare September 17, 2023 19:48

This was referenced Sep 18, 2023

Next City Name Cycles When Build City Dialog Opened but City Not Built #424

Open

Adding river deltas. Algorithm needs to be checked #431

Closed

benskywalker reviewed Sep 23, 2023

View reviewed changes

pcen requested a review from WildWeazel September 25, 2023 02:08

pcen added 27 commits October 1, 2024 00:41

fix rebase issue

d29b8d3

implement grid layer

d3a1d24

simplify unit selection with null coalescing operator

40b1413

rendering terrain

75d4b9e

load fog of war pcx

ac0ddbe

reveal fog when units move

db9a2ec

z indices

190a17e

use Z index readonly for city scene

94a8d71

terrain and tiles wrap horizontally, not cities or units

fae72b0

clearer enum name, use enum values instead of booleans

1cc87b8

wrap units

a1cc504

do wrap checks in Camera

9c6eefd

wrap horizontally with fewer map jumps

32af1f0

horizontally wrap cities

caed09a

dry up wrap offset code

9a651b7

clean up

e40969b

fix left wrap

0122964

simpler predicate functions, less error prone state in camera

15ed46e

add tile leeway for wrapping to hide temporary map tear

8ca14c6

only check for wrapping when camera moves, better naming, comment mai…

70e0b35

…n wrap function

remove unused code

41bca4f

fix issue where getVisibleWorld initially uses invalid GlobalCanvasTr…

f446a12

…ansform value

unit and tile IDs, having issues with updating save json when changin…

7ef3427

…g format

remove debug print

4f78192

pcen force-pushed the pcen/use-tilemaps branch from 62c2795 to 4f78192 Compare October 1, 2024 04:45

WildWeazel added the help wanted Extra attention is needed label Dec 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[godot4] Use Godot TileMap for Rendering #407

[godot4] Use Godot TileMap for Rendering #407

pcen commented Jun 21, 2023 •

edited

Loading

pcen commented Jul 28, 2023

QuintillusCFC commented Jul 31, 2023

QuintillusCFC commented Jul 31, 2023

pcen commented Jul 31, 2023

QuintillusCFC commented Aug 3, 2023

pcen commented Aug 7, 2023

WildWeazel commented Aug 10, 2023

pcen commented Aug 10, 2023

WildWeazel commented Aug 10, 2023

pcen commented Aug 11, 2023

WildWeazel Sep 4, 2023

pcen Sep 17, 2023

WildWeazel Sep 4, 2023

WildWeazel Sep 4, 2023

pcen Sep 5, 2023

WildWeazel Sep 4, 2023

pcen Sep 5, 2023

WildWeazel Sep 4, 2023

pcen Sep 5, 2023

WildWeazel left a comment

benskywalker Sep 23, 2023

pcen Sep 23, 2023

WildWeazel commented Oct 12, 2023

WildWeazel commented Dec 31, 2024

[godot4] Use Godot TileMap for Rendering #407

Are you sure you want to change the base?

[godot4] Use Godot TileMap for Rendering #407

Conversation

pcen commented Jun 21, 2023 • edited Loading

pcen commented Jul 28, 2023

QuintillusCFC commented Jul 31, 2023

QuintillusCFC commented Jul 31, 2023

pcen commented Jul 31, 2023

QuintillusCFC commented Aug 3, 2023

pcen commented Aug 7, 2023

WildWeazel commented Aug 10, 2023

pcen commented Aug 10, 2023

WildWeazel commented Aug 10, 2023

pcen commented Aug 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WildWeazel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WildWeazel commented Oct 12, 2023

WildWeazel commented Dec 31, 2024

pcen commented Jun 21, 2023 •

edited

Loading