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

Register enum type names in release build #64253

Merged
merged 1 commit into from Jan 5, 2023

Conversation

heppocogne
Copy link
Contributor

This commit fixes #64084

  • Add script error for non-existing C++ enum types.
  • Modify macro definitions for release build.

@anvilfolk
Copy link
Contributor

Can you fix the conflicts? I think this is the only way to fix #69532 :)

@heppocogne
Copy link
Contributor Author

@anvilfolk
Fixed confliction.

@anvilfolk
Copy link
Contributor

Thank you! You also need to squash your commits into a single commit! :)

@heppocogne
Copy link
Contributor Author

ok, I squashed 2 commits into a commit.

@anvilfolk
Copy link
Contributor

Perfect! I struggled a lot to make that work the first time! :) Let's see if we can't get this PR in :)

@akien-mga
Copy link
Member

You still have two commits, because you merged the master branch back on top of your squashed commit instead of rebasing:
image

Moreover, the commit message should not be "Squash commit" - that's what you did, but we need to know what the commit does. It should describe how it improves/changes/fixes engine functionality. See https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-messages-with-readability-in-mind

@anvilfolk
Copy link
Contributor

Sorry, my bad for not spotting that. That's what you get with insomnia at 5am ;)

This guide helped me: https://www.sitepoint.com/git-interactive-rebase-guide/

@akien-mga akien-mga changed the title Fix native enum release 1 Register enum type names in release build Dec 19, 2022
@heppocogne
Copy link
Contributor Author

@akien-mga @anvilfolk
Thanks for your help. I could squash all commits into one commit.

@vnen
Copy link
Member

vnen commented Dec 29, 2022

I've actually been meaning to do this, since the alternative is remove the ability to address enums by their names. The only thing I want to see is a comparison in size of release builds with and without this. Though I doubt it would be much since I don't think that are that many enums around.

Comment on lines 381 to 383
#ifdef DEBUG_METHODS_ENABLED

#define BIND_CONSTANT(m_constant) \
::ClassDB::bind_integer_constant(get_class_static(), StringName(), #m_constant, m_constant);

Copy link
Member

Choose a reason for hiding this comment

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

Given this is the same with or without DEBUG_METHODS_ENABLED it could stay outside the #ifdef too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that's probably because I made mistakes when I tried to fix conflicts.
And unfortunately, my branch seems to be broken now (cannot checkout or reset). I will close this PR and make new one later if necessary.

@heppocogne heppocogne requested review from a team as code owners December 29, 2022 13:31
@YuriSizov YuriSizov requested review from a team and removed request for a team December 29, 2022 14:44
@YuriSizov
Copy link
Contributor

Hey, @heppocogne, you need to rebase and squash again. You seem to have dragged unrelated changes into your PR, probably by merging master into it. Remember that in feature branches you shouldn't merge master, you should rebase those branches against the master instead.

@heppocogne
Copy link
Contributor Author

@YuriSizov Thank you! I could manage to restore my repository.

@vnen vnen requested a review from a team December 30, 2022 13:17
@vnen
Copy link
Member

vnen commented Dec 30, 2022

@neikeq yeah, it would be interesting to profile that, maybe we can optimize it. That could potentially help with editor startup as well.

Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

Would be interesting to profile startup to see the actual impact, but I believe this change is needed in any case to keep consistency between debug and release behavior.

@akien-mga akien-mga merged commit 1816f49 into godotengine:master Jan 5, 2023
@akien-mga
Copy link
Member

Thanks!

@neikeq
Copy link
Member

neikeq commented Jan 5, 2023

If this has a negative on startup time, perhaps we could cache the result from enum_qualified_name_to_class_info_name into a static field. The downside of being static is that it's kept alive until the program finishes, but honestly, adding it to ClassDB is almost the same in practice.

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.

GDScript 2.0: An enum type annotation is parsed differently in release and debug/editor mode
7 participants