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

Small touch-up for scripts in general #11

Merged
merged 1 commit into from
Jan 14, 2019
Merged

Small touch-up for scripts in general #11

merged 1 commit into from
Jan 14, 2019

Conversation

YeldhamDev
Copy link
Contributor

  • Use preload instead of load.
  • Remove unnecessary passes.
  • Make correct use of "_" in the beginning of variables.

Assets/UI/Scripts/ExitScene.gd Show resolved Hide resolved
Assets/UI/Scripts/Main/CloseMenu.gd Show resolved Hide resolved

func _pressed():
var _stfu = get_tree().change_scene_to(scene)
Copy link
Contributor

@aaronfranke aaronfranke Jan 13, 2019

Choose a reason for hiding this comment

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

The _ is used to suppress the "unused variable" warnings in 3.1 compared to var stfu.

(Without the var _stfu = part entirely, Godot 3.1 gives warnings about an unused return value from change_scene_to)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we even gonna use this variable for anything? Because this method can be called directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Godot 3.1 gives warnings about an unused return value from change_scene_to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then #warning-ignore:return_value_discarded should be used. Makes more sense than creating a variable just to silence the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And with that, another thing that somehow I didn't noticed before: Why each button has its own script? The menu scene should have a script that holds all of these methods.

Copy link
Contributor

@aaronfranke aaronfranke Jan 13, 2019

Choose a reason for hiding this comment

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

#warning-ignore:return_value_discarded is longer to write than var _stfu = and does the same thing.

Having a "Button Manager" script would go against object-oriented design, but could be done I think. I'd suggest asking @LinuxDonald what his preferred programming paradigm is. I don't know if there's a standard in Godot for managers vs self-managed, but I do know that in Godot each node is supposed to only perform one function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#warning-ignore:return_value_discarded is longer to write than var _stfu = and does the same thing.

That's still creating a variable just to silence a warning! Besides being far more explicit, if there's a system to properly ignore warnings, that should be used instead of this.

I'd suggest asking @LinuxDonald what his preferred programming paradigm is.

Considering that he's just starting to learn Godot as far as I'm aware, I don't think that would help that much, but I could give it a shot.
However, most projects that I've seen (including official ones: 1, 2) just connect the buttons to the main script, as it's much more manageable than hopping through scripts for such small tasks.

Copy link
Contributor

@aaronfranke aaronfranke Jan 14, 2019

Choose a reason for hiding this comment

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

Good point.

@aaronfranke aaronfranke merged commit c71e36a into unknown-horizons:master Jan 14, 2019
@YeldhamDev YeldhamDev deleted the general_script_touchup branch January 14, 2019 01:50
@aaronfranke aaronfranke added the code Programming code and game logic label Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Programming code and game logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants