search

C7-Game / Prototype

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

171 - Fix the City Label Blinking #339

Closed QuintillusCFC closed 1 year ago

QuintillusCFC commented 2 years ago

Closes #171

This is split into two commits. The first one just extracts the city layer from the map layer so it's easier to see what's specifically related to cities.

The second has the actual fix. You probably want to just look at that commit to tell what's changed.

Credit to Xrayez for figuring out how to fix this.

Verification of this should include running Development to confirm you have the issue (the prevalence seems to depend on hardware and perhaps memory use), and then running this branch to confirm it fixes the issue. E.g. I can re-create the issue reliably on my desktop but it was much rarer on my laptop..

Kright commented 2 years ago

The cityLabel texture being class-level prevents it from being garbage collected, but also results in it being shared for all cities, with the same dimensions/color.

Yep. As I understand, godot collects all "DrawTextureRectRegion" operations and do them in one GPU call. At time of GPU call all "textures" are the same, because it is just one texture reused. For the same reason Texture shouldn't be deleted too early.

I suggest to add 

private List<ImageTexture> cache = new List<>();

as class field and clear this cache in OnBeginDraw(). So each ImageTexture will outlive previous texture render.

In drawObject for each city new texture should be created and added to cache;

WildWeazel commented 2 years ago

is anyone still looking into this?

QuintillusCFC commented 2 years ago

I went on vacation for 15 days, so I wasn't then, but I can finish this up this week. Maybe today but I'm mostly doing a first pass on everything (including totally non-civ-related-stuff) first before going deeper into anything.

But basically I agree with Kright that we'll have to cache them, though probably using a Dictionary rather than a List.

QuintillusCFC commented 2 years ago

Fixed up the cache issue (commit 6547a56), and also did some refactoring to break up the city label drawing into a few constituent parts. Finally, rebased on the latest development branch so that when testing this you'll see both this issue and the seams-in-the-map issue fixed.

WildWeazel commented 2 years ago

Sadly I'm still seeing the issue, perhaps a bit less but after some zooming and panning I can get them flashing regularly even on turn 1.

QuintillusCFC commented 2 years ago

@WildWeazel Do you also see the issue for this commit? 40dfdc7 (#339) That was the status at the time @Kright reviewed earlier.

I'll try again on my desktop, maybe something with the Dictionary is causing issues. Hopefully it's a minor oversight in the changes since commit 40dfdc7, if it's a problem with the theory in general, we would need a new theory.

(Even before the changes, the frequency of the issue varied highly by machine, which is why I can't try it now on my laptop and have a reasonable expectation of whether it's fixed or not - it was rare enough before on the laptop that a lack of the problem might just be good luck)

WildWeazel commented 2 years ago

No, I can't seem to reproduce it with 40dfdc7

QuintillusCFC commented 1 year ago

All right, just tried it again both natively on my desktop, and in the C7 VM. I'm afraid that with the latest commit, in neither case do I see any city label blinking. :dunno: I played 135 turns in the most recent test so there were lots of cities, double-checked the commit in GitKraken, scrolled and zoomed and panned, and didn't see the problem at all, although I did note that as you scroll left or right, occasionally the rightmost tile is not drawn; we are likely slightly too aggressive in chopping the viewport down to size.

There might still be a hardware-specific configuration item in why @WildWeazel still sees it; as I noted earlier, I could reproduce the original issue (before these changes) fairly easily on my desktop, but not on my laptop. That probably means WildWeazel will have to be the one to investigate it farther, due to having the hardware to reproduce the issue.

Unfortunately 40dfdc7 being merged is not a valid option as it causes all city labels to be the same size, so the options are to not merge at all, or merge with all the changes. I'd still favor the latter as it sounds like these changes fixed it for some cases and didn't make it worse in others.

My hardware setup:

Laptop (rarely had issue at all): i7-8750H (6-core), GTX 1050 Mobile, 8 GB DDR4 Desktop (had issue regularly but tolerably, these changes fix it): i5-2500K, RX 480 8 GB, 20 GB DDR3 VM: Running on desktop with C7 Development image

WildWeazel commented 1 year ago

My older GPU (GTX 760) could be the culprit, though I'll have to check if I enabled graphics acceleration in my VM

QuintillusCFC commented 1 year ago

Hmm, could be. I wonder if it is related to GPU acceleration in the VM specifically? I had GPU acceleration disabled in the VM when I tried this, and haven't tried it with GPU acceleration since the shared folder went away when I turned it on.

The only older nVIDIA GPU I have is from 2007 and is definitely flakey, so I can't really compare-test that... could probably dust off my old HD 6870 Radeon, but that's a bit of an apples-to-oranges comparison.

QuintillusCFC commented 1 year ago

I'm going to go ahead and merge this since it's been sitting for a month and it has an approval and at least for me, fixes the issue. @WildWeazel , if you want to investigate it farther you can, but since I can only reproduce it without these changes, I can't do anything else about it.