Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Visual upgrade + code changes #56

Merged
merged 27 commits into from
Jul 4, 2022
Merged

Conversation

b7g
Copy link
Contributor

@b7g b7g commented Jun 28, 2022

Description

This PR includes a visual and a code makeover.

Visual

splash screen
n1

new menu
fix1

new info dialog
fix2

new section about/contribute/report a bug
fix3

new section settings
fix4

the new ingame pause menu
n6

Changes, changes everywhere

Additionally to the visual changes depicted above following changes were made:

  • added new Utils Singleton (for commonly used utilities)
  • function handle_error was moved from GameManager -> Utils
  • added new UserSettings singleton (loads, saves and handles the user settings)
  • overhauled the GameManager singleton quite a bit
  • added new PlayerManager singleton (split from old GameManager)
  • bbcode support for game descriptions was removed for good
  • pause_menu got an overhaul as well
  • translations for the main app were added, translations for the games and descriptions still need to be done.

Type of change

  • Bug fix (non breaking change that fixes an issue)
  • New feature (non breaking change that adds functionality)
  • Tooling (non breaking change that adds functionality to the workflow)
  • --- Breaking change (fix or feature that causes existing functionality to not work as expected)

Environment

  • OS: Ubuntu 22.04 LTS
  • Software versions: Godot 3.4

Testing

tested everything at some point, should work

Checklist

  • My code follows the general style guidelines of the project
  • I have perfomed a self-review of my code
  • I have commented my code, particulary where it is unclear
  • My changes generate no new warnings or errors
  • The project compiles and runs correctly

Copy link
Contributor

@RedstoneMedia RedstoneMedia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you need an image of a black/white square png image. Just use a color rect, if you need the spacing, just use containers (margin) for that.

@RedstoneMedia
Copy link
Contributor

Also, please capitalize the first character in a sentence and words on buttons (example, "spielen" instead of "Spielen")

@Numenter
Copy link
Contributor

Windows error:
data_manager.gd:41 -> error ERR_CANT_CREATE -> "user://savefiles/p/res://games/flappybird/"

Why is game_id a path? "res://games/flappybird"

@RedstoneMedia
Copy link
Contributor

Also _last_played in game_card.gd cannot be of type int, since get_time_last_played can return null, which causes an immediate crash, if the game has not been played yet.

Using a static video instead of animations in Godot is also a problem, on larger monitors and not that easy to edit.

Another thing to note is, that not every game has a high score, so it should not be displayed in the game info.

@Joshix-1
Copy link
Contributor

0 shouldn't be the default for score in end_game. There should be the option for games without scores. (null as default)

@Numenter
Copy link
Contributor

Windows error:
WebM"s stutter on windows and the support for WebM is removed in 4.0
godotengine/godot#55815
Theora is recommended

@Numenter
Copy link
Contributor

Why would you need an image of a black/white square png image. Just use a color rect, if you need the spacing, just use containers (margin) for that.

The black square imange is used in Settings/Boot Splash, which has to be an image.
The white square imange is used in the blur shader.

@Numenter
Copy link
Contributor

Numenter commented Jun 28, 2022

Not a fan of the cryptic translation codes.
image
image

T_PAUSED, T_RESUME, T_RESTART, T_BACK_TO_MENU would be a better fit.

PS: here is a PR for this b7g#1

@Joshix-1
Copy link
Contributor

Yes translation codes should always contain the whole english text, then you know when you have to update all the other langauge variations

Copy link
Contributor

@Joshix-1 Joshix-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looked over the code and noticed some problems

game/app/scenes/game_card.gd Outdated Show resolved Hide resolved
game/app/scenes/game_card.gd Outdated Show resolved Hide resolved
return _last_played


func _format_playtime(playtime) -> Dictionary:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like how inaccurate this is

Copy link
Contributor Author

@b7g b7g Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is an improvement
Before

  • it was always displayed in seconds
  • it had 3 decimal places

The target audience of this application are not measurement engineers right?

game/app/singleton/game_manager.gd Outdated Show resolved Hide resolved
game/app/singleton/game_manager.gd Outdated Show resolved Hide resolved
game/app/singleton/game_manager.gd Outdated Show resolved Hide resolved
extends Node


func get_current_player() -> String:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why move that here, if it still has no functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a. why should the GameManager handle the players, even if it is not really functional yet?
b. de-clutters the GameManager

game/menu/README.md Outdated Show resolved Hide resolved
game/translations/tr.csv Outdated Show resolved Hide resolved
@b7g
Copy link
Contributor Author

b7g commented Jun 28, 2022

Thanks for the lots of comments and info, I appreciate it.
Seems like shit hit the fan a bit haha.

I will respond to the comments first and then resolve the issues.

@b7g
Copy link
Contributor Author

b7g commented Jun 28, 2022

Also, please capitalize the first character in a sentence and words on buttons (example, "spielen" instead of "Spielen")

I think you meant it the other way around in your example?

@b7g
Copy link
Contributor Author

b7g commented Jun 28, 2022

Windows error: data_manager.gd:41 -> error ERR_CANT_CREATE -> "user://savefiles/p/res://games/flappybird/"

Why is game_id a path? "res://games/flappybird"

shouldn't be, you are right
I'm a little confused there I have to say, as I haven't really touched the data_manager at all.

@b7g
Copy link
Contributor Author

b7g commented Jun 28, 2022

Also _last_played in game_card.gd cannot be of type int, since get_time_last_played can return null, which causes an immediate crash, if the game has not been played yet.

Using a static video instead of animations in Godot is also a problem, on larger monitors and not that easy to edit.

Another thing to note is, that not every game has a high score, so it should not be displayed in the game info.

a. correct
b. I don't see how an more complex animation can be easily recreated with code. Also other games use videos too, so there should be at least a way to prevent the issues, if they are present right now.
c. just for clarification: just the games that have no score are meant?

@b7g
Copy link
Contributor Author

b7g commented Jun 28, 2022

0 shouldn't be the default for score in end_game. There should be the option for games without scores. (null as default)

I agree

@b7g
Copy link
Contributor Author

b7g commented Jun 28, 2022

Windows error: WebM"s stutter on windows and the support for WebM is removed in 4.0 godotengine/godot#55815 Theora is recommended

Will be changed

@b7g
Copy link
Contributor Author

b7g commented Jun 28, 2022

Not a fan of the cryptic translation codes.

T_PAUSED, T_RESUME, T_RESTART, T_BACK_TO_MENU would be a better fit.

PS: here is a PR for this b7g#1

I agree on this, but had problems with the sizing in the past.

  • Godot did not allow smaller buttons then ones the translation codes fit into. Maybe that is fixed by now or we can work around that by always using containers.

@RedstoneMedia
Copy link
Contributor

The black square imange is used in Settings/Boot Splash, which has to be an image.

I'm pretty sure there is an option to set a custom boot splash screen instead of just an image. If that is not the case just make the image a 1x1 transparent image and make the background black ( this would also only appear very shortly before a splash screen scene is loaded ).

The white square imange is used in the blur shader.

And you can't create a rectangle in a shader ?

@RedstoneMedia
Copy link
Contributor

RedstoneMedia commented Jun 28, 2022

Also, please capitalize the first character in a sentence and words on buttons (example, "spielen" instead of "Spielen")

I think you meant it the other way around in your example?

Yeah sorry my bad. The other way around is obviously correct.

@RedstoneMedia
Copy link
Contributor

c. just for clarification: just the games that have no score are meant?

Ideally yes. However this would require another manual entry in the game CFGs. That's why hiding the highscore for all games until a non null highscore is returned by end_game, is also a good option.

@Numenter
Copy link
Contributor

The black square imange is used in Settings/Boot Splash, which has to be an image.

I'm pretty sure there is an option to set a custom boot splash screen instead of just an image. If that is not the case just make the image a 1x1 transparent image and make the background black ( this would also only appear very shortly before a splash screen scene is loaded ).

A 1x1 transparent image works fine for the boot splash and it can be used for the blur shader too.

The white square imange is used in the blur shader.

And you can't create a rectangle in a shader ?

Maybe, I'm not a shader guy.

@b7g
Copy link
Contributor Author

b7g commented Jun 28, 2022

I'm pretty sure there is an option to set a custom boot splash screen instead of just an image. If that is not the case just make the image a 1x1 transparent image and make the background black ( this would also only appear very shortly before a splash screen scene is loaded ).

There is not. Will change to pixel.

And you can't create a rectangle in a shader ?

I just did it like in the example project provided by Godot.
Maybe that is possible, but my knowledge with shaders is lacking.

next level visual - Translation keycodes changed
@RedstoneMedia
Copy link
Contributor

RedstoneMedia commented Jun 28, 2022

Maybe that is possible, but my knowledge with shaders is lacking.

I did it without the image file. Turns out all you have to do is change the TextureRect to a ColorRect. The color of the rect doesn't matter since its overwritten anyway by the shader.

The PR: b7g#3

@b7g
Copy link
Contributor Author

b7g commented Jun 29, 2022

When you need to highlight something in the description, it just means you haven’t done your job properly and are bringing it together with highlights in the description on the cost of the user.

@ASecondGuy
Copy link
Contributor

When you need to highlight something in the description, it just means you haven’t done your job properly and are bringing it together with highlights in the description on the cost of the user.

The Description isn't to bandaid over flaws of the game, I agree. If someone uses it like that you should tell them to stop and fix the game.
The description is to describe the game (mind bending concept I know). That description can sometimes be improved with the options bbcode offers. I see no downsides to including it. (Again, someone misusing a tool is not a reason to remove the tool)

b7g and others added 3 commits June 30, 2022 15:21
@b7g
Copy link
Contributor Author

b7g commented Jun 30, 2022

Please note that I swapped out the images in the description to reflect the latest changes.
The changes are rather subtle, therefore the heads-up.

@Numenter
Copy link
Contributor

Bug:
Crash on Soriting : Last Played when not all games have been played

Reproduce:

  1. Remove savegames
  2. Sort by last played

ERROR menu.gd:144 Invalid operands 'Nil' and 'Nil' in operator '>'.

Could be the same bug @RedstoneMedia stated before.

Also _last_played in game_card.gd cannot be of type int, since get_time_last_played can return null, which causes an immediate crash, if the game has not been played yet.

@b7g
Copy link
Contributor Author

b7g commented Jun 30, 2022

should be finally fixed :')

@b7g
Copy link
Contributor Author

b7g commented Jun 30, 2022

@Joshix-1

I made the formatting of the playtime more precise.

  • now also seconds are displayed if minutes were not reached yet
  • when the playtime is between 1 - 10 hours also the minutes are displayed in the popup dialog

maybe that's precise enough for your liking?

@ASecondGuy
Copy link
Contributor

I finished the scene tree refactor of this.
Just need to configure the theme so it looks like it did before.

@Joshix-1
Copy link
Contributor

Sounds better. Could we maybe show even more precise times on hover? x days y hours z minutes

@RedstoneMedia
Copy link
Contributor

RedstoneMedia commented Jun 30, 2022

Just need to configure the theme so it looks like it did before.

You mean how it looks now right ? You don't want to change the look back to before my (and b7g's) improvements. Shouldn't you be able to just apply the theme (instead of changing it) ?

A button is going to stay a button, how would a scene tree refactor, change this ?

@ASecondGuy
Copy link
Contributor

Just need to configure the theme so it looks like it did before.

You mean how it looks now right ? You don't want to change the look back to before my (and b7g's) improvements. Shouldn't you be able to just apply the theme (instead of changing it) ?

A button is going to stay a button, how would a scene tree refactor, change this ?

I improved the scene tree structure and removed unnecessary nodes.
Because there were still theme overwrites used and the theme didn't have all nodes or they were configured differently it looks like crap.
I'm now updating the theme to get the intended look. (I'm not gonna search through old files to find the exact numbers and colors)
I'm not much of an artist so someone probably needs to go over the colors again.
The buttons stated exactly the same btw.

@b7g
Copy link
Contributor Author

b7g commented Jun 30, 2022

Sounds better. Could we maybe show even more precise times on hover? x days y hours z minutes

It’s common for playtime to be displayed in hours, not minutes or days

@RedstoneMedia
Copy link
Contributor

RedstoneMedia commented Jun 30, 2022

@ASecondGuy

I'm not gonna search through old files to find the exact numbers and colors

Okay ? That's a bit suspicious.

Edit: From what I can see, only the top bar looks really off right now. That could be handled with overrides (Or a new custom theme just for the menu bar)

Because there were still theme overwrites used

Also theme overrides are not the devil (If its only used once or twice)

Anyways, your current branch has some obvious issues (I know WIP and stuff but still):

  • About.gd needs to be renamed to about.gd
  • The label assertion in About.gd always fails on my system (Not just in weird instances. It can not be safely ignored)

@ASecondGuy
Copy link
Contributor

@RedstoneMedia What is suspicious about that? Most of the visuals were in the theme overwrites before.
I reorganized the nodes and therefore most visuals went missing. I don't wanna search for them so I approximated.
Btw. the menu bar is a PanelContainer and gets its visuals directly from the main_theme.

And I know overwrites aren't inherently bad. But I found at least a handful of them in V/HBoxContainers where they make limited sense. And I removed many of those Containers.

ASecondGuy and others added 2 commits July 2, 2022 18:57
* menu scene tree refactor game page

* menu scene tree refactor about page

* menu scene tree refactor settings page

* fix scroll to bug report

* Fixing stuff the merge broke

* Fix title font and added margin container to main_theme.tres

* fix about page crashing on reload

* game_card scene tree refactor

* game_card_add_yours scene tree refactor

* fix assert fail

* fix themes and theme overwrites

* fix merge

* rename About.gd to about.gd

* fix about page empty bottom part

* Fixed most colors/margins

* fix visuals

* Disable bbcode for no reason

* fix theme even more

* Fixed more margins/color issues

* add much needed about.gd docu & make it respond to line spacing

* fix visuals again

* refactor about.gd thanks to Numenters tip

* Fixed play time game card position

* fix more visuals

Co-authored-by: RedstoneMedia <34373974+RedstoneMedia@users.noreply.github.com>
Comment on lines -32 to +27
"base": "MarginContainer",
"base": "Reference",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort_it.gd extends form MarginContainer and my godot is changing this back to MarginContainer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is weird, my godot always changes it back and forth. I honestly have no Idea what causes this.

@@ -86,7 +86,7 @@ class ScoreSorter:


## Handle data for the game that ended.
func game_ended(player: String, game_id: String, start_time, score: Dictionary):
func game_ended(player: String, game_id: String, start_time, score_or_null):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't like the name. Is it score or is it null or is the score null?
Just call it score and add a comment with which values are expected.

Copy link
Contributor

@Numenter Numenter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two little things, but you got my 👍 anyway.

Copy link
Contributor

@ASecondGuy ASecondGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this is a good PR I can get behind.

@letsgamedev letsgamedev merged commit c7dff1c into letsgamedev:main Jul 4, 2022
@b7g b7g deleted the next-level-visual branch October 25, 2023 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants