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

Next level visual fix #6

Merged
merged 32 commits into from
Jul 2, 2022
Merged

Conversation

ASecondGuy
Copy link

@ASecondGuy ASecondGuy commented Jun 30, 2022

Description

refactoring of the scene tree.

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)

Testing

I ran the game multiple times and opened/closed many windows/games.

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

@ASecondGuy
Copy link
Author

ASecondGuy commented Jun 30, 2022

The merge broke stuff.
I'm gonna fix it real quick.

@ASecondGuy
Copy link
Author

It's fixed.
Everything ready to be merged.

@ASecondGuy
Copy link
Author

@RedstoneMedia I think this is everything. Could you take a look at it?

@RedstoneMedia
Copy link

RedstoneMedia commented Jun 30, 2022

Ok so the assertion error is now fixed, and the files are named correctly.
However:

  • You have way too small margins (especially in the game card)
  • You added a border to things that previously haven't had one (borders aren't always bad, but it doesn't look good in this case)
  • Some colors are now to dark (game card background). Menu bar background is now too light
  • Second, darker color on the bottom of the game card info pop-up was removed by you
  • Font size (And type) was changed for some components (not the game cards)

Currently, I don't see any other issues than this

Edit: Removing basically all margin containers (And overrides that could create these margins) sure cuts down on the complexity of the scene tree, however in this case, maybe that wasn't the best option.

@ASecondGuy
Copy link
Author

Thanks for the feedback.
PanelContainers have a content margin build in so MarginContainers were redundant where I removed them.
The rest is all just visual stuff.
I could improve the looks (and probably will) but I think it's better if someone with a feeling for that would do it right the first time.

@ASecondGuy
Copy link
Author

@RedstoneMedia I went and searched for the old values.
I can barely distinguish the 2 versions now.
I think this is good enough. Looking back I should have done this right from the start.

@ASecondGuy
Copy link
Author

@b7g This is ready to be merged

@RedstoneMedia
Copy link

RedstoneMedia commented Jul 1, 2022

Some of the colors and margins are still not exactly right. But this is definitely better (Also thanks for ignoring this: ASecondGuy#7 and then creating your own commit to the thing you said you wouldn't fix 😀)

@ASecondGuy
Copy link
Author

Some of the colors and margins are still not exactly right. But this is definitely better (Also thanks for ignoring this: ASecondGuy#7 and then creating your own commit to the thing you said you wouldn't fix 😀)

Sorry. I just saw that this morning. That's on me.

@b7g
Copy link
Owner

b7g commented Jul 1, 2022

You are trolling me right?
I will not merge this.

GETTA YO FUXKEN BBCODE OUTTA THERE

also

In the current state it destroys the visual appearance at many places

  • it has 2 links in blue, one in white
  • one link does not work
  • line-spacing is gone
  • margins are gone at many places

@Numenter
Copy link

Numenter commented Jul 1, 2022

Im looking into the about:_process()
"_write_to_label" fails in "_notification" and "set_text" only in the beginning.
a "_write_to_label" in "_ready" fixs that.

Im not able to break it after this.
image

@ASecondGuy
Copy link
Author

Last time I checked it also fails when the scene changes. Ready is not called in that case.

@RedstoneMedia
Copy link

  1. There is a gap between header and body

To fix this just set the separation, of the root menu node to 0 (currently not overridden)

the game info popup right section lost it’s white space

Set the VC stretch ratio under the InfoSection (In the game card) to 1 instead of 0.4 (Who even did that ?)

the game info popup lost it’s shadow

  • Replace the empty style box with a StyleBoxFlat.
  • Unset draw center.
  • Set the shadow size to 6 and the color to #22000000

I think these changes are so simple that I won't even have to make a PR for this.

@Numenter
Copy link

Numenter commented Jul 1, 2022

Last time I checked it also fails when the scene changes. Ready is not called in that case.

I drilled some more and "set_text" is triggered on scene change. ( text == "" and val == 'old text content' ).
So nothing changes at runtime, can be ignored.
It happens because text is an export and the scene value differs from default. Is some engine woodoo.

There is a comment that it fails in editor. I cant reproduce that.

@ASecondGuy
Copy link
Author

Set the VC stretch ratio under the InfoSection (In the game card) to 1 instead of 0.4 (Who even did that ?)

I did that. We both fixed the same problem at the same time from different sides. That overcompensated.

The first link had "" while the others didn't. That might be why it didn't work for you @b7g please confirm that it works now.

@Numenter thanks for looking into that. I'll try to fix it asap.

@b7g
Copy link
Owner

b7g commented Jul 2, 2022

Yes, the link works now.

still … “refactoring of the scene tree.”:
aaa-1500

It’s awful.

The line spacing in the about tab is still missing too

@RedstoneMedia
Copy link

RedstoneMedia commented Jul 2, 2022

If you also mean the change of the "Spielzeit" location on the game card from bottom to top:
I actually think that it looks better when it's aligned on the top.

Your comparison does show some differences, however I think it's important to state that not every change is automatically a flaw (The user will only see one version and won't be able to nitpick the difference s like we are doing right now)

@b7g
Copy link
Owner

b7g commented Jul 2, 2022

Yes, I agree.
Still it’s not just a “refactoring of the scene tree.” and feels like a downgrade visually.

To be clear: The playtime location is on the bottom with this PR and it looks terrible.

To merge this I will also require:

  • that line-spacing in the about section is added again

It would be nice, if:

  • the spacing after the logo badge would be added again
  • the line spacing in the description of the game card and game card info popup is added again

I know it’s terrible hard for you to move your finger for a few pixels, but things were placed with consideration.
And you are breaking through that like an elephant in a flowerbed.

@RedstoneMedia
Copy link

Please keep your personal issues with others out of this thread, as it doesn't help the current discussion.
Also, who are you even referring to ?

Btw.: @ASecondGuy I opened a PR to fix the play time text location ASecondGuy#8

@ASecondGuy
Copy link
Author

Just as I finished it too...
Btw. what is the problem with the linespacing.
I don't know what is supposed to be wrong with that.

@b7g
Copy link
Owner

b7g commented Jul 2, 2022

You refers to the PR

@ASecondGuy
Copy link
Author

You refers to the PR

Please be nice to the PR. It might not have a brain or a body or a consciousness or fingers it could move but it does have feelings

@RedstoneMedia
Copy link

RedstoneMedia commented Jul 2, 2022

Just as I finished it too...

Bruh.

Ff this goes on we need to actually start to coordinate our actions (This requires a reliable and fast form of communication (Not GitHub or Discord)). I don't like wasting my time

@b7g
Copy link
Owner

b7g commented Jul 2, 2022

Bildschirmfoto vom 2022-07-02 18-02-43

@RedstoneMedia
Copy link

Where does it even look like that?

@ASecondGuy
Copy link
Author

It works on my machine ;)
image
Please send a real screenshot I can't look at your screen in case you didn't know.

@b7g
Copy link
Owner

b7g commented Jul 2, 2022

It’s exaggerated…
I don’t expect that you (the PR) have any knowledge about Typography.

linespacing
left is this PR

@b7g b7g merged commit 8e271ad into b7g:next-level-visual Jul 2, 2022
b7g added a commit that referenced this pull request Jul 2, 2022
@b7g
Copy link
Owner

b7g commented Jul 2, 2022

thanks 🏈

@RedstoneMedia
Copy link

🏈

1 similar comment
@ASecondGuy
Copy link
Author

🏈

@ASecondGuy ASecondGuy deleted the next-level-visual-fix branch July 4, 2022 12:15
b7g pushed a commit that referenced this pull request Jan 29, 2023
…5-additions

Sort It physics godot3.5 fix additions
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.

4 participants