C7-Game / Prototype

An early-stage, open-source 4X strategy game
https://c7-game.github.io/
MIT License
32 stars 9 forks source link

[godot4] Use Godot TileMap for Rendering #407

Open pcen opened 1 year ago

pcen commented 1 year ago

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:

https://github.com/C7-Game/Prototype/pull/408 adds units to this branch - merged https://github.com/C7-Game/Prototype/pull/412 adds cities - merged https://github.com/C7-Game/Prototype/pull/419 adds grid view - merged https://github.com/C7-Game/Prototype/pull/421 adds fog of war - merged https://github.com/C7-Game/Prototype/pull/422 adds horizontal wrapping - merged

pcen commented 11 months ago

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

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

I think that definitely makes sense, this branch is essentially the base for tilemaps https://github.com/C7-Game/Prototype/pull/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 commented 11 months ago

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

pcen commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

WildWeazel commented 9 months ago

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

pcen commented 9 months ago

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

Just merging at this point, however the only thing missing from this branch to achieve visual parity with the current rendering is unit hit point indicators.

QuintillusCFC commented 8 months ago

Checking out this branch and godot4, there are a couple other items beyond hitpoints that are different vis-a-vis visual partity with godot4. One is rivers, which is going to be covered by https://github.com/C7-Game/Prototype/pull/433. The other I noticed is the city display.

Godot 3:

image

Godot 4:

image

(Note the darker shading around the city, but that isn't specific to tilemaps. Also the width is too long, and the '3' isn't centered. But these aren't tilemap-specific, just noting them as something to address before 3.5.x is deprecated)

Tilemaps:

image

For some reason the "Chariot: 10" now escapes its container.

The changes are getting close... tilemaps and Godot 4 are both a lot of work.