godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.83k stars 21.14k forks source link

Theme types cannot have characters such as . or / #97074

Open FireCatMagic opened 1 month ago

FireCatMagic commented 1 month ago

Tested versions

Godot v4.4.dev unknown - Windows 10.0.19045 - OpenGL 3 (Compatibility) - NVIDIA GeForce RTX 4050 Laptop GPU (NVIDIA; 32.0.15.6070) - 13th Gen Intel(R) Core(TM) i5-13500HX (20 Threads)

System information

Godot v4.4.dev unknown - Windows 10.0.19045 - OpenGL 3 (Compatibility) - NVIDIA GeForce RTX 4050 Laptop GPU (NVIDIA; 32.0.15.6070) - 13th Gen Intel(R) Core(TM) i5-13500HX (20 Threads)

Issue description

Theme types cannot contain characters such as periods (.) or slashes (/)

I couldn't find information about this on the documentation but I could just be blind. I think it would be cleaner if you could use names with periods as if they were sub-classes of the main type.

Steps to reproduce

  1. Create a theme
  2. Set a name such as Button.Test
  3. See it does not add

Minimal reproduction project (MRP)

N/A

AThousandShips commented 1 month ago

This is because they have to be valid names, see here, should be documented in Theme.add_type

(This issue should track when the improvement to the editor is merged I'd say and the docs can be tracked separately)

Calinou commented 1 month ago

The editor should have validation added for this, so that it prevents you from clicking the Add button with an error label displaying.

FireCatMagic commented 1 month ago

This is because they have to be valid names, see here, should be documented in Theme.add_type

(This issue should track when the improvement to the editor is merged I'd say and the docs can be tracked separately)

Makes sense. Though I'm curious as to why though, when the theme types names themselves aren't node names. There's probably a good reason that I'm unaware of

AThousandShips commented 1 month ago

They are meant to be valid class names I'd say, ~unsure where it's checked exactly~

It's checked here: https://github.com/godotengine/godot/blob/99a7a9ccd60fbe4030e067b3c36d54b67737446d/scene/resources/theme.cpp#L178-L186

FireCatMagic commented 1 month ago

They are meant to be valid class names I'd say, ~unsure where it's checked exactly~

It's checked here:

https://github.com/godotengine/godot/blob/99a7a9ccd60fbe4030e067b3c36d54b67737446d/scene/resources/theme.cpp#L178-L186

image I was playing with it and it's even stricter than String.validate_node_name

Probably is the ascii identifier function

Theme types (supposedly) don't work like node or class names so I am confused on the purpose of this still - in my head it seems like it would make sense for them to allow any StringName I understand for ones that map to base classes such as &"Button" being the name of an actual class, but not all theme types need to map to actual classes

AThousandShips commented 1 month ago

Probably is the ascii identifier function

Yes as you can see in the code it is that exact one

They should be class names though as they are designed to be used with class names generally