Apply type hints to example scripts to avoid triggering any GDScript warnings or errors

[nlupugla]

Previously, several of the scripts that users were meant to edit in a custom layer were not fully statically typed. This meant that a user with strict static typing set up in their project settings would be bombarded with errors upon opening these scripts. My goal is to statically type all of the user-facing scripts so that even a user with the strictest possible GDScript settings can edit the Dialogic example scripts without an issue. So far, I have typed the visual novel example. I will continue with the other example scenes, but I wanted to create this draft to make sure I'm on the right track and this work is appreciated.

[Jowan-Spooner]

Thanks for your PR!

Firstly (I don't have any strong feelings on this), but why make all implicit static typing explicit? The gdscript styleguide says: Include the type hint when the type is ambiguous, and omit the type hint when it's redundant. I'm only referring to places where I felt the type was already implied enough (e.g. @export var enabled := true). I'm not having the warning on right now, so I'm not sure if this is a needed change or a preference thing.

Secondly, I'm not too happy about:

• Changing the LayoutLayers /LayoutBase to extend Control and CanvasLayer instead of Node. I think this limits the possibilities quite a bit, so if there is any way around it, I would prefer that.
• Introducing the methods to get subsystems (on the DialogicUtil). We are planning on finding a way to make the subsystems statically typed but this seems like a very bad solution and at most a temporary fix. I would rather leave those changes out for now, and add a better fix in another PR.

[nlupugla]

Thanks for the feedback! When I make edits, do you prefer I use amend commits so there is only one commit in the PR or have each round of changes be its own commit?

Firstly (I don't have any strong feelings on this), but why make all implicit static typing explicit

If it were code for my personal project, I'd totally agree with you on the implicit typing. However, my approach was to turn on all the errors I could in the project settings in order to accommodate users with a range of possible error settings.

Changing the LayoutLayers /LayoutBase to extend Control and CanvasLayer instead of Node

I think I found a way around this using Object's dynamic set(property, value) function :)

Introducing the methods to get subsystems (on the DialogicUtil).

That's fair, I can use a different approach for now. Are you okay with the way I gave the subsystems class_names like DialogicSubsystemHistory? If not, I can use the dynamic set approach I mentioned above.

[Jowan-Spooner]

When I make edits, do you prefer I use amend commits so there is only one commit in the PR or have each round of changes be its own commit?

Feel free to add more commits, it's easier to follow your changes that way. We squash the PR into one commit when merging.

If it were code for my personal project, I'd totally agree with you on the implicit typing. However, my approach was to turn on all the errors I could in the project settings in order to accommodate users with a range of possible error settings.

That makes sense. Feel free to continue doing it then. It's a bit annoying to deviate from the gdscript style guide there, but it would indeed be nice if people can use these scripts whatever error settings they have on.

think I found a way around this using Object's dynamic set(property, value) function :)

Perfect!

Are you okay with the way I gave the subsystems class_names like DialogicSubsystemHistory? If not, I can use the dynamic set approach I mentioned above.

Yeah sure you can give them names, although they might change a bit or we might remove them again later on, not sure yet.

[nlupugla]

I've finished adding static types to the other examples and implemented your suggestions. Happy new year!

[nlupugla]

Did a bit more thorough testing on my PR and realized that I broke a few things by replacing some uses of e.g.

%Title.something

with

@onready var title : Label = %Title

#later

title.something

because certain properties and overrides seem to be set before the _ready callback. I'll try and fix it by replacing these @onready variables with getters that return a node of the appropriate type.

[coppolaemilio]

if it worked before the refactor and now you need to create all those getters, I feel like the PR is not an improvement (at least in that area)

I'm all in for adding types to variables, but I feel like the PR is growing a bit out of proportion.

[nlupugla]

Hi Emi, thanks for the feedback!

To be clear, this is what a user with GDScript set to error on unsafe property/method access sees when they try making a custom layer without this PR:

Note that this happens even when the user has checked "Exclude Addons" option in their GDScript settings because making a custom layer creates scripts in the user's directory.

This PR aims to solve all that those error messages by replacing statements like

%DialogicNode_DialogicText.aligment

with a type-safe alternative. I don't like bloating scripts with trivial getters any more than the next dev, but it seemed like the best option among what I could think of. Here are the options I've considered. If you think there's an option I missed that would be better, I'm happy to implement it :).

1. inline cast

(%DialogcNode_DialogicText as DialogicNode_DialogicText).alignment

pros: very simple
cons: very verbose

2. @onready variable

# towards the top of the script...
@onready var dialogic_text : DialogicNode_DialogicText = %DialogicNode_DialogicText

# later on in a method body...
    dialogic_text.alignment

pros: simple
cons: breaks code that executes before the Node is _ready (such as _apply_export_overrides )

3. local variable

# in a method body...
var dialogic_text : DialogicNode_DialogicText = %DialogicNode_DialogicText
dialogic_text.alignment

pros: simple
cons: annoying to repeat these local variable declarations at the top of each method that needs them

4. property with getter

# towards the top of the script...
var dialogic_text : DialogicNode_DialogicText : get : return %DialogicNode_DialogicText

# later on in a method body...
    dialogic_text.alignment

pros: concise
cons: creates potentially misleading behavior e.g.

var new_node : DialogicNode_DialogicText = DialogicNode_DialogicText.new()
dialogic_text = new_node
dialogic_text == new_node # returns false because dialogic_text == %DialogicNode_DialogicText

5. pure getter

# towards the top of the script...
func get_dialogic_text() -> DialogicNode_DialogicText:
    return %DialogicNode_DialogicText

# later on in a method body...
    get_dialogic_text().alignment

pros: clear
cons: bloats scripts with trivial getters

[nlupugla]

Congrats on getting the automated unit tests working! It seems like maybe there is a file I should add to my branch to make the CLI happy?

[CakeVR]

Congrats on getting the automated unit tests working! It seems like maybe there is a file I should add to my branch to make the CLI happy?

Try rebasing/merging the latest changes from main to your branch.

[nlupugla]

Thanks! Hopefully I did that correctly.

[nlupugla]

Sorry for the duplicated commits. I tried rebasing and there were some merge conflicts, which I'm still kind of nooby at resolving. I think HEAD is where I want it nevertheless.

[nlupugla]

Reverted all changes to Dialogic Subsystems as per suggestions of @CakeVR :)

[coppolaemilio]

what's the difference here? I remember adding the is_empty check before. Not sure why (or maybe I'm just not remembering correctly). @Jowan-Spooner do you recall?

[nlupugla]

Oh, I forgot I made this change. It's completely irrelevant to the rest of the PR, so I'll revert it. It just came when I was playing around with the code to make sure I understood it well enough :)

[coppolaemilio]

@nlupugla left a couple comments. Overall I think it should be good. The code looks super ugly now, but I guess there's not much we can do about it. Waiting for @Jowan-Spooner approval before merging 👍

[nlupugla]

Thanks Emi! Really appreciate the review :) I'm sorry for butchering everyone's code with all the type hints, gets, sets, and calls. At least some of it will get cleaned up when the Dialogic subsystems get class_names.

[Jowan-Spooner]

@nlupugla It looks good overall. Why did you add static tpying to a bunch of dialogic-nodes and to some base scripts? Is that necessary? Those shouldn't be copied to the user directory I think... From my understanding this would only have to be on the layer scripts, wouldn't it?

I would prefer to keep this aggressive static typing contained to where it's REALLY necessary.

[nlupugla]

Totally agreed! I'll check again some time today that all the changes are necessary. Thanks for all the help and feedback :)

[Jowan-Spooner]

I'm not certain but I would assume you could get rid of the changes in:

• node_portrait_container.gd
• node_dialog_text.gd
• node_name_label.gd
• node_next_indicator.gd
• node_type_sound.gd

And also

• dialogic_layout_base.gd
• dialogic_layout_layer.gd

[nlupugla]

This kind of pattern is my solution to avoid horrible lines like
((DialogicUtil.autoload().get(&'Input') as Object).get(&'auto_skip') as Object ).get(&'enabled')
It does mean that what used to be a one-liner: DialogicUtil.autoload().Input.auto_skip.enabled is now three lines. If my solution is still too ugly, I can revert it for now and try to coordinate with the subsystem type PR.

[Jowan-Spooner]

Thanks! Glad this is done for now. Hope the statically typed subsystems make it in soon.

[nlupugla]

Awesome! Thanks for all the help and discussion :)