Godot 4 - Githubissues

pcen commented 1 year ago

requires .NET 7 to build and run

This branch may not build without errors if you check it out in an existing local repo and try opening it in the Godot 4 rc2 editor. Either cleaning the project and rebuilding from scratch or cloning a fresh copy should then build without error.

Marked as ready for review because the PR is already at ~2,400 lines changed, and as far as I can tell, gameplay features all work, and there are only a few graphics issues that can be fixed in smaller follow up PRs before making another release. Notably, those graphics issues are:

unit action buttons have a weird pixelated effect when hovered over instead of changing colour
some swap terrain tiles have some black pixels where it should be blue
the texture rect cities are drawn on leaves a faint transparent grey rectangle overlayed on the surrounding terrain I have not checked if all of these were introduced in porting to Godot 4, some might be present in Godot 3.5.

A significant portion of the diff (my guess is about half) was done by the Godot tool for upgrading projects form 3 to 4, or from manually renaming classes that had their name changed in Godot 4 but the upgrade tool missed. The only significant change I made was rewriting the unit animations to use Godot.AnimatedSprite2D instead of using custom shaders, since the shaders were broken in Godot 4, and this approach has the added benefit of enabling unit animations to also be created in the Godot editor.

QuintillusCFC commented 1 year ago

Well, this is interesting!

pcen commented 1 year ago

currently compiles and runs, but with the following runtime error:

ERROR: System.IO.FileNotFoundException: Could not load file or assembly 'Serilog, Version=2.0.0.0, Culture=neutral, PublicKeyToken=24c2f752a8e58a10'. The system cannot find the file specified.

File name: 'Serilog, Version=2.0.0.0, Culture=neutral, PublicKeyToken=24c2f752a8e58a10'
   at LogManager._Notification(Int64 what)
   at Godot.Object.InvokeGodotClassMethod(godot_string_name& method, NativeVariantPtrArgs args, Int32 argCount, godot_variant& ret) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/Object.cs:line 848
   at Godot.Node.InvokeGodotClassMethod(godot_string_name& method, NativeVariantPtrArgs args, Int32 argCount, godot_variant& ret) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/Node.cs:line 1793
   at LogManager.InvokeGodotClassMethod(godot_string_name& method, NativeVariantPtrArgs args, Int32 argCount, godot_variant& ret) in /Users/paul/dev/Prototype/C7/Godot.SourceGenerators/Godot.SourceGenerators.ScriptMethodsGenerator/LogManager_ScriptMethods_Generated.cs:line 31
   at Godot.Bridge.CSharpInstanceBridge.Call(IntPtr godotObjectGCHandle, godot_string_name* method, godot_variant** args, Int32 argCount, godot_variant_call_error* refCallError, godot_variant* ret) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/CSharpInstanceBridge.cs:line 24

None of the csproj files reference Serilog 2.0.0.0 and I think all of the other Serilog extensions target the correct version

pcen commented 1 year ago

Fixed the assembly not found error by adding true to the csproj file

Serilog targets 2.0.0.0 assembly when using version 2.12.0, so it's not a versioning issue seems related to https://github.com/godotengine/godot/issues/42271

pcen commented 1 year ago

Creating ImageTextures via ImageTexture.CreateFromImage seems to render transparent pixels as magenta in the current release candidate. Created https://github.com/godotengine/godot/issues/73128 as I could not find any documentation or configuration to fix rendering from code.

Edit: seems like an issue with the pcx parsing code and not godot, although only visible with godot 4

Edit 2: pcx parsing is fine, if I hardcode the RGB values of the last two colours in a pcx colour pallet, it will affect the colour rendered around the terrain textures. For example, setting the last to colours to be RGBA(0,0,0,0) (and using a white clear colour so it's visible), terrain tiles are rendered like this:

so it seems like the default is to use alpha blending instead of alpha test

QuintillusCFC commented 1 year ago

It's worth noting that Civ3 PCXs have a few oddities, namely around transparency. IIRC the last two colors are treated as transparent, and traditionally one (or both?) of those is magenta.

The relevant code is likely to be in PCXToGodot.cs. Notably in the loadPalette method, we have this block:

        for (int i = CIV3_TRANSPARENCY_START; i < 256; i++) {
            ColorData[i] &= 0x00ffffff;
        }

        if (shadows) {
            for (int i = 240; i < 256; i++) {
                ColorData[i] = ((255 - i) * 16) << 24;
            }
        }

CIV3_TRANSPARENCY_START is set to 254.

We're essentially doing our own texture importing since Godot doesn't have PCX support, so my guess is something in the color spaces has changed in Godot 4.

QuintillusCFC commented 1 year ago

I'm curious where you are in terms of getting it running. I downloaded Godot 4 RC 1 and checked out the branch, and the main menu loads, but when I try to start a game it fails. It says:

[ERR] 23:37:02 Game: Unexpected error in Game.cs _Ready 
System.NotSupportedException: The type 'System.Int32[,]' is not supported. The unsupported member type is located on type 'C7GameData.GameMap'. Path: $.gameData.map | LineNumber: 4 | BytePositionInLine: 12.

The stacktrace points to at C7GameData.C7SaveFormat.Load(String path) in C:\Development\Civ Related Projects\Prototype - G4\C7GameData\SaveFormat.cs:line 69, which is where it loads the static save file.

Trying to re-generate the static save file (after pointing it to my local copy of the source save file) fails because in Util.cs (the QueryCiv3 one), as it notes:

        // NOTE: .NET 5 and later require a Nuget package and Encoder registration for these older encoding pages

We must have been re-generating it as needed from .NET 4.7.2. It might be time to bite the bullet on that one too.

But I'm not sure if that is what you are seeing or not. I suspect you may have a game loading since you mentioned units rendering but looking weird in one of the commit messages.

QuintillusCFC commented 1 year ago

As a side note, other than my existing PR (https://github.com/C7-Game/Prototype/pull/384), I'm trying to stick to non-front-end changes, like AI, since front end changes may require additional changes for Godot 4 compatibility.

pcen commented 1 year ago

It's worth noting that Civ3 PCXs have a few oddities, namely around transparency. IIRC the last two colors are treated as transparent, and traditionally one (or both?) of those is magenta.

The relevant code is likely to be in PCXToGodot.cs. Notably in the loadPalette method, we have this block:
      for (int i = CIV3_TRANSPARENCY_START; i < 256; i++) {
          ColorData[i] &= 0x00ffffff;
      }

      if (shadows) {
          for (int i = 240; i < 256; i++) {
              ColorData[i] = ((255 - i) * 16) << 24;
          }
      }
CIV3_TRANSPARENCY_START is set to 254.

We're essentially doing our own texture importing since Godot doesn't have PCX support, so my guess is something in the color spaces has changed in Godot 4.

Yeah I was messing around with the RGB values of the colours where the alpha value is masked out, and it seems like Godot still blends those colours despite alpha being 0.

pcen commented 1 year ago

I'm curious where you are in terms of getting it running. I downloaded Godot 4 RC 1 and checked out the branch, and the main menu loads, but when I try to start a game it fails. It says:
[ERR] 23:37:02 Game: Unexpected error in Game.cs _Ready 
System.NotSupportedException: The type 'System.Int32[,]' is not supported. The unsupported member type is located on type 'C7GameData.GameMap'. Path: $.gameData.map | LineNumber: 4 | BytePositionInLine: 12.
The stacktrace points to at C7GameData.C7SaveFormat.Load(String path) in C:\Development\Civ Related Projects\Prototype - G4\C7GameData\SaveFormat.cs:line 69, which is where it loads the static save file.

Trying to re-generate the static save file (after pointing it to my local copy of the source save file) fails because in Util.cs (the QueryCiv3 one), as it notes:
        // NOTE: .NET 5 and later require a Nuget package and Encoder registration for these older encoding pages
We must have been re-generating it as needed from .NET 4.7.2. It might be time to bite the bullet on that one too.

But I'm not sure if that is what you are seeing or not. I suspect you may have a game loading since you mentioned units rendering but looking weird in one of the commit messages.

I have not seen this issue. I've been loading the checked-in map without issues, but I have not tried generating one. I have noticed that it's hard to "clean" a Godot project, so I might try cloning a fresh copy of the repo and checking out the Godot 4 branch before opening in the editor - I think I had to delete my local copy and do this at some point in the porting process.

pcen commented 1 year ago

waiting on https://github.com/godotengine/godot/issues/73601

pcen commented 1 year ago

I'm curious where you are in terms of getting it running. I downloaded Godot 4 RC 1 and checked out the branch, and the main menu loads, but when I try to start a game it fails. It says:
[ERR] 23:37:02 Game: Unexpected error in Game.cs _Ready 
System.NotSupportedException: The type 'System.Int32[,]' is not supported. The unsupported member type is located on type 'C7GameData.GameMap'. Path: $.gameData.map | LineNumber: 4 | BytePositionInLine: 12.
The stacktrace points to at C7GameData.C7SaveFormat.Load(String path) in C:\Development\Civ Related Projects\Prototype - G4\C7GameData\SaveFormat.cs:line 69, which is where it loads the static save file.

Trying to re-generate the static save file (after pointing it to my local copy of the source save file) fails because in Util.cs (the QueryCiv3 one), as it notes:
        // NOTE: .NET 5 and later require a Nuget package and Encoder registration for these older encoding pages
We must have been re-generating it as needed from .NET 4.7.2. It might be time to bite the bullet on that one too.

But I'm not sure if that is what you are seeing or not. I suspect you may have a game loading since you mentioned units rendering but looking weird in one of the commit messages.

I updated my .net from 6 to 7 and started seeing this error. I updated the QueryCiv3 Util and added the nuget package for .net > 5. I don't know why I wasn't seeing this error before, because I also get it when I try to load now with .net 6 but it seems like the default json serialisation cannot handle multidimensional arrays

Edit: I added a converter for 2d arrays, and added a new static map json, but it should also be possible to create a new one using _Console again

WildWeazel commented 1 year ago

Aside from not being able to open the project in Godot 4 so far, this is too large for me to fully review, but I'm skimming to see what kinds of things have changed.

QuintillusCFC commented 1 year ago

Took a look at this again; read a comment on the Godot blog suggesting having a fresh directory for Godot 4 projects, so this time I re-cloned into a new directory, and opened it with Godot 4.

It runs!

And it plays.

There are some lingering issues, mainly around fonts and transparency (although only isolated cases for transparency). I've tried to capture as many of these issues as possible in the following screenshot:

My thought is we should create a Godot4 branch and merge this into that new branch, leaving Development as Godot 3 in the short term. This has a few benefits, which are somewhat tradeoffs:

Allows us to get this branch merged somewhere so the future updates can be small fixes, as @pcen mentioned
Related to the above, makes it feasible to review those changes individually.
Since Carthage is arriving soon(TM), gives us the option of releasing Carthage on Godot 3 in case some of those visually small fixes wind up being more complex than expected.

There is a downside as well:

Front end visual changes would have to be applied to both Develop and Godot4 until we fully migrate

But considering most of our changes are back-end logic (even things like improving exploration by taking mountains and hills into account will be logic updates), I think that's acceptable in the short term.

Hmm, it does also mean that we'd have to synchronize the branches even for the backend ones though, since it's all one monorepo.

Thoughts?

(note: I tested this on my Ryzen 5800H laptop with a Radeon 6600 GPU and Windows 10. But I think the real reason it worked this time was the freshly cloned repo)

WildWeazel commented 1 year ago

I haven't been attending to this since as @QuintillusCFC pointed out we still need to release Carthage proper. According to the blog they expect 4.1 to be stable by mid-summer, so I'd expect our first Dutch release to target 4.1, especially as "The engine-wide focus for 4.1 will be on stability, usability, and performance." I don't know if any of these planned rendering changes are relevant to the issues here.

Also, I never figured out what was going on with the VM, so I installed 4.0.2 and loaded this branch and the first time opening the project it crashed my display driver so... yeah.

pcen commented 1 year ago

I created a godot4 branch that is this branch rebased on Development: https://github.com/C7-Game/Prototype/tree/godot4 I'll close this branch, and godot 4 changes should target godot4 as the base branch. I'll try to keep the godot4 branch up to date with Development, but the godot 4 API is fairly different, so it will be easier to keep up to date if we prioritise "backend" changes in Development

C7-Game / Prototype

Godot 4 #387