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

Check if property exists before tweening #81525

Merged
merged 1 commit into from Nov 9, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Sep 10, 2023

When you try to tween a non-existent property, you get a confusing message about type mismatch between "nil" and target value type.
This PR adds a debug check whether a property exists, to print a clearer error.

@KoBeWi KoBeWi added this to the 4.2 milestone Sep 10, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner September 10, 2023 20:26
@bitsawer
Copy link
Member

I think adding a check with a better error message is good, but I'm a bit cautious about adding behavior that changes between debug and release builds. Could we do this check in both debug and release builds? It seems like a useful thing to do and it doesn't seem to be too heavy from performance perspective, either. And people often have to debug release build error logs too, so it also helps there.

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 10, 2023

The behavior is the same (unless your final value is null for some reason...), only the error message is different. But indeed the check is cheap, so maybe it can be in release too, idk. The only time when it's relevant is when a) you don't test your project (this error is easy to catch) b) do some unsafe stuff with _get_property_list().

@timothyqiu
Copy link
Member

The variable should not be named p_value as it's not a parameter :P

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 11, 2023

It's property value xd

@akien-mga
Copy link
Member

I'd suggest using prop_value to avoid confusion.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

I think it's fine like that, and we can always change it if someone signals that they need this check in production builds as well.

@akien-mga akien-mga merged commit 0e6160a into godotengine:master Nov 9, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the tweening_the_impossible branch November 9, 2023 19:16
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.

None yet

5 participants