Godot-Community-Games-Org / game1

First community game let's go!
GNU General Public License v3.0
22 stars 5 forks source link

Added UI Node #11

Closed ChaoticTechSupport closed 2 weeks ago

ChaoticTechSupport commented 3 weeks ago
Yonodactyl commented 3 weeks ago

For the sake of transparency, I don't know if we are ready to merge this in just yet.

I know it's nothing crazy, and it's really just setting some stuff up for once we get moving, but we are still hoping to have some standards established before we start working on code. I say we keep this PR opened, but it should hang around until we establish our processes.

StrawberrySmoothieDev commented 3 weeks ago

For the sake of transparency, I don't know if we are ready to merge this in just yet.

I know it's nothing crazy, and it's really just setting some stuff up for once we get moving, but we are still hoping to have some standards established before we start working on code. I say we keep this PR opened, but it should hang around until we establish our processes.

Agreed. I'd say keep this in the request stage until we better figure out out baselines and foundation. For all we know we could be using 3D ui and this would just be bloat. I'll take a closer look at the code whenever I can to judge cleanliness but otherwise not much to do here.

KaizNike commented 2 weeks ago

Not robust enough for my liking. Works well on a single Label, resizes well on Y axis, some cutoff on X. When I tried a more complex UI structure it didn't do anything for it due to the fact it requires each and every control node's font to be manually resized. Where most times I set the font size on a parent node and let that dictate what size the children are, but it's close to greatness!

ChaoticTechSupport commented 2 weeks ago

Not robust enough for my liking. Works well on a single Label, resizes well on Y axis, some cutoff on X. When I tried a more complex UI structure it didn't do anything for it due to the fact it requires each and every control node's font to be manually resized. Where most times I set the font size on a parent node and let that dictate what size the children are, but it's close to greatness!

did you try the other modes? also, do yo mean you use theme and not theme override? because I can actually activate theme mode in the code if that's what your talking about.

KaizNike commented 2 weeks ago

Yeah I meant theme support for this text resize UX. I got it working for a moment on my own, but its prone to errors. For example if for any reason there is a non-Control child it crashes.

Here are my changes to UserInterface.gd to review:

func _ready() -> void:
    # Store font size and node
    for node: Node in UtilityFunctions.get_all_Children(self):
        if not node.is_class("Control"):    #Safety
            continue
        if node.has_theme_font_size_override("font_size"):
            font_nodes[node] = node.get("theme_override_font_sizes/font_size")
        elif self.theme is Theme:
            if self.theme.has_default_font_size() and node.get_theme_default_font_size():   #Addition to cover themes
                font_nodes[node] = node.get_theme_default_font_size()

    get_tree().get_root().size_changed.connect(font_resize)
    font_resize()

What do you think?

ChaoticTechSupport commented 2 weeks ago

Yeah I meant theme support for this text resize UX. I got it working for a moment on my own, but its prone to errors. For example if for any reason there is a non-Control child it crashes.

Here are my changes to UserInterface.gd to review:

func _ready() -> void:
  # Store font size and node
  for node: Node in UtilityFunctions.get_all_Children(self):
      if not node.is_class("Control"):    #Safety
          continue
      if node.has_theme_font_size_override("font_size"):
          font_nodes[node] = node.get("theme_override_font_sizes/font_size")
      elif self.theme is Theme:
          if self.theme.has_default_font_size() and node.get_theme_default_font_size():   #Addition to cover themes
              font_nodes[node] = node.get_theme_default_font_size()

  get_tree().get_root().size_changed.connect(font_resize)
  font_resize()

What do you think?

i just got back and finished in 5 minutes subbmitting for review now.

KaizNike commented 2 weeks ago

I think it's looking perfectly up to par now.

EDIT: Noticed the focus start doesn't work as expected for TabContainers (for those that use em like me) as the focus is set to an internal node.

ChaoticTechSupport commented 2 weeks ago

I think it's looking perfectly up to par now.

EDIT: Noticed the focus start doesn't work as expected for TabContainers (for those that use em like me) as the focus is set to an internal node.

thanks for telling me! I'll look into that and hopefully have it fixed soon.

ChaoticTechSupport commented 2 weeks ago

I think it's looking perfectly up to par now.

EDIT: Noticed the focus start doesn't work as expected for TabContainers (for those that use em like me) as the focus is set to an internal node.

hi! it is now fixed! if you want to take a look see if somthings off that would be great!

StrawberrySmoothieDev commented 2 weeks ago

Taking a look for a approval. Think this one is general enough to be approved even now.

ChaoticTechSupport commented 2 weeks ago

Looks pretty good, made some suggestions, however, with some testing I might recommend making it work not just on viewport resize but also on the resizing of the container/parent control node. Also is there any reason for making it work via a parent and not directly in the label? Thanks! :3

Hi Smoothie, this setup is meant to ensure the game looks consistent on any monitor. It's not designed to actively resize containers. If you're looking for text resizing within containers, that's a different feature from what I've implemented. Resizing containers instead of adjusting the viewport wouldn't make sense, as both methods would give the same result. However, container resizing would be less effective, as it could break animations and take up more storage space.

if you have a specific idea which you would want container based text resizing then i would implament it as a seperate feature but otherwise i dont see the use.

KaizNike commented 2 weeks ago

Looks pretty good, made some suggestions, however, with some testing I might recommend making it work not just on viewport resize but also on the resizing of the container/parent control node. Also is there any reason for making it work via a parent and not directly in the label? Thanks! :3

Hi Smoothie, this setup is meant to ensure the game looks consistent on any monitor. It's not designed to actively resize containers. If you're looking for text resizing within containers, that's a different feature from what I've implemented. Resizing containers instead of adjusting the viewport wouldn't make sense, as both methods would give the same result. However, container resizing would be less effective, as it could break animations and take up more storage space.

if you have a specific idea which you would want container based text resizing then i would implament it as a seperate feature but otherwise i dont see the use.

I agree, container text resizing isn't in the scope of this rather small but useful script.

StrawberrySmoothieDev commented 2 weeks ago

Looks pretty good, made some suggestions, however, with some testing I might recommend making it work not just on viewport resize but also on the resizing of the container/parent control node. Also is there any reason for making it work via a parent and not directly in the label? Thanks! :3

Hi Smoothie, this setup is meant to ensure the game looks consistent on any monitor. It's not designed to actively resize containers. If you're looking for text resizing within containers, that's a different feature from what I've implemented. Resizing containers instead of adjusting the viewport wouldn't make sense, as both methods would give the same result. However, container resizing would be less effective, as it could break animations and take up more storage space.

if you have a specific idea which you would want container based text resizing then i would implament it as a seperate feature but otherwise i dont see the use. Noted. Looks good to me :3