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

Add shorthand for using singleton string names #81303

Merged
merged 1 commit into from
May 13, 2024

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Sep 4, 2023

Follow-up to #80573

This PR adds CoreStringName() and SceneStringName() macros as a shorthand for CoreStringNames::get_singleton()-> and SceneStringNames::get_singleton()->. It makes the singleton string names easier to use and code is cleaner.

I also applied some minor cleanup:

  • removed some stringname singleton includes from header files
  • removed unused stringnames: ___pdcdata, __getvar
  • renamed some stringnames to be less confusing (they used underscore for member, but not for string, e.g. _script for "script"

For the last point I had to make some exceptions, because the actual name conflicted with a C++ keyword. I changed the name from _name to name_ (up to discussion), to make it more clear that underscore is not part of the name.

My plan after that is to replace all usage of "strings" and SNAME("strings") with their singleton equivalent if it exists already. Right now they are used inconsistently.

Another thing is that some names are used only once. I think they can be replaced with SNAME(); it didn't exist when they were created.

Production edit: closes godotengine/godot-roadmap#3

@KoBeWi KoBeWi added this to the 4.x milestone Sep 4, 2023
@KoBeWi KoBeWi requested review from a team as code owners September 4, 2023 15:10
@KoBeWi KoBeWi force-pushed the the_forbidden_name_of_strings branch from 5417652 to 8c55241 Compare September 4, 2023 15:52
@fire fire changed the title Add shorthand for using singleton string names Add a shorthand for using singleton string names Sep 4, 2023
@fire fire changed the title Add a shorthand for using singleton string names Add shorthand for using singleton string names Sep 4, 2023
@KoBeWi KoBeWi force-pushed the the_forbidden_name_of_strings branch from 8c55241 to c354dcb Compare September 6, 2023 13:41
@KoBeWi KoBeWi force-pushed the the_forbidden_name_of_strings branch 2 times, most recently from d2b32fa to 5140ec1 Compare September 17, 2023 17:06
@KoBeWi KoBeWi force-pushed the the_forbidden_name_of_strings branch 2 times, most recently from 67ca79c to 0e4907d Compare November 25, 2023 20:49
@KoBeWi KoBeWi force-pushed the the_forbidden_name_of_strings branch from 0e4907d to 3805efd Compare December 25, 2023 15:35
@KoBeWi KoBeWi force-pushed the the_forbidden_name_of_strings branch 4 times, most recently from 59200d8 to 056ce4a Compare February 14, 2024 20:47
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it mostly works as expected.

This reduces binary size by about 8 KB too (optimized Linux x86_64 editor).

106,980,384  godot.linuxbsd.editor.x86_64  <-- This PR
106,988,576  godot.linuxbsd.editor.x86_64.master

However, I get a series of errors when exiting the editor. This occurs on any project, even an empty project:

ERROR: Attempt to disconnect a nonexistent connection from '@CreateDialog@5292:<CreateDialog#505883418985>'. Signal: 'confirmed', callable: 'CreateDialog::_confirmed'.
   at: _disconnect (./core/object/object.cpp:1416)
ERROR: Attempt to disconnect a nonexistent connection from '@CreateDialog@10229:<CreateDialog#775476516600>'. Signal: 'confirmed', callable: 'CreateDialog::_confirmed'.
   at: _disconnect (./core/object/object.cpp:1416)
ERROR: Attempt to disconnect a nonexistent connection from '@CreateDialog@6763:<CreateDialog#575894745070>'. Signal: 'confirmed', callable: 'CreateDialog::_confirmed'.
   at: _disconnect (./core/object/object.cpp:1416)
ERROR: Attempt to disconnect a nonexistent connection from '@CreateDialog@6344:<CreateDialog#556684831719>'. Signal: 'confirmed', callable: 'CreateDialog::_confirmed'.
   at: _disconnect (./core/object/object.cpp:1416)
ERROR: Attempt to disconnect a nonexistent connection from '@CreateDialog@6197:<CreateDialog#550175271553>'. Signal: 'confirmed', callable: 'CreateDialog::_confirmed'.
   at: _disconnect (./core/object/object.cpp:1416)
ERROR: Attempt to disconnect a nonexistent connection from '@EditorFileSystem@3:<EditorFileSystem#28269610226>'. Signal: 'filesystem_changed', callable: 'EditorDirDialog::reload'.
   at: _disconnect (./core/object/object.cpp:1416)
ERROR: Attempt to disconnect a nonexistent connection from '@CreateDialog@4543:<CreateDialog#471121025568>'. Signal: 'confirmed', callable: 'CreateDialog::_confirmed'.
   at: _disconnect (./core/object/object.cpp:1416)
ERROR: Attempt to disconnect a nonexistent connection from '@CreateDialog@4361:<CreateDialog#462698862674>'. Signal: 'confirmed', callable: 'CreateDialog::_confirmed'.
   at: _disconnect (./core/object/object.cpp:1416)

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 14, 2024

I can't reproduce the errors. Are you sure they are caused by this PR? Any chance for their stack trace?

@KoBeWi KoBeWi force-pushed the the_forbidden_name_of_strings branch from 056ce4a to a262d2d Compare May 11, 2024 16:53
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I like the change, and it seems to work fine on my end from a quick test.

Would be good if @Calinou could test again and see if he reproduces the error about missing signal connections.

@akien-mga akien-mga requested a review from Calinou May 11, 2024 19:43
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master bdc0316), it works as expected now. I can't reproduce the errors on exit anymore.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 May 12, 2024
@akien-mga akien-mga merged commit ba533f5 into godotengine:master May 13, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the the_forbidden_name_of_strings branch May 13, 2024 10:14
StringName changed;
StringName _script;
StringName script;
Copy link
Member

@AThousandShips AThousandShips May 13, 2024

Choose a reason for hiding this comment

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

Introduced a conflict with:

Writing up a fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants