Conversation
There was a problem hiding this comment.
Thanks! This makes sense to me. There's no need for script to be stored twice, especially since the Variant based storage can be less efficient and more error prone than the reference based one.
Would be nice to get another voice from one more GDScript / C# maintainer, but I also don't see huge risks in this. The property has existed since the first commit. It's likely simply no longer needed.
|
We should especially carefully check |
|
I did look at PlaceHolderScriptInstance when making the PR. To me it looks like they are always created with a valid script. Not that familiar with tool mode. What might be a pitfall? Doesn't tool mode create normal script instances? |
dsnopek
left a comment
dsnopek
left a comment
There was a problem hiding this comment.
Thanks!
This looks good to me. I'm glad this is also removing set_script_and_instance() - when working on PR #102373 it was also noted that this method isn't really needed.
There is one potential risk here: if there are any GDExtensions providing scripting languages that depend on the Ref<Script> getting cached on the Godot side, and won't expect get_script() to get called so often. However, I think at worst this would be a performance issue, and not one that breaks compatibility? So, I think it should be fine
|
It could be compat breaking for an implementation that tries to obtain the script from the Still it technically breaks compat in a way that should at least be documented. |
dalexeev
left a comment
dalexeev
left a comment
There was a problem hiding this comment.
Looks good to me too, great catch! Even if we encounter some regression, we can always fix it or revert the change.
raulsntos
left a comment
raulsntos
left a comment
There was a problem hiding this comment.
The C# changes look good to me.
|
Regarding tool mode, I did try this with my kanban board addon and on a cursory look everything seems to work alright. |
|
Thanks! |
7 participants
Removes 24 bytes from
Objectsince the script was stored asVariant. Instead obtains the script from theScriptInstance.As far as I can see
Object::get_scriptis not used in any obvious hot paths, so the additional overhead should not matter that much.In addition to making object smaller, this is also makes inconsistent states impossible.