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

Rework variable tracker module #153

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

worron
Copy link
Collaborator

@worron worron commented Dec 12, 2023

First of all, thank you for sharing your work, this tool is amazing. Unfortunately my project has a bit tricky structure, sometime there is not a main scene loaded, only singletons placed in the tree. Current implementation of variable_tracker can't handle such situations, so I made a deep rework of the module for my needs. Please take a look at the code, may be you will be interested in approving it into your project.

Main changes:

  • Proper handle situations when current scene root node not found.
  • Auto load singletons detection based on project settings.
  • Auto tracking interval now exposed via module settings.
  • More detailed tip messages in console.

Module refactored to handle situation where no current active
scene available, only singletons in the tree. Singleton detection
changed to use project settings data. Current scene tracking loop
delay moved to module settings and now can be configured by user.
@Ark2000
Copy link
Owner

Ark2000 commented Dec 13, 2023

Thank you for your contribution! seems it is a much improved version, I'll check it out later.

Copy link
Owner

@Ark2000 Ark2000 left a comment

Choose a reason for hiding this comment

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

Great work! The only thing needs to be changed is the typed loop for for compatibility reasons. It will be merged after that.

## Find the root node of current active scene.
func get_scene_root() -> Node:
# Assuming current scene is the first node in tree that is not autoload singleton.
for node:Node in core.get_tree().root.get_children():
Copy link
Owner

Choose a reason for hiding this comment

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

Seems it's a new feature of Godot4.2, however we want to keep backwards compatibility, please use syntax like this to simulate a typed for loop:

for _node in xxx:
  var node:Node = _node
  # ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I think we can simply go without typed loop variable then.

func _check_autoloads() -> void:
var _user_singleton_names := []

for node:Node in core.get_tree().root.get_children():
Copy link
Owner

Choose a reason for hiding this comment

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

same review comment as previous one.

if core.module_manager.has_module(SHELL_MODULE_NAME):
var ishell = core.module_manager.get_module(SHELL_MODULE_NAME)
ishell.interactive_shell.output(message)

Copy link
Owner

Choose a reason for hiding this comment

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

print something to dev console should be improved later in 1.9.0 to avoid unnecessary setup.

return true

return false

Copy link
Owner

Choose a reason for hiding this comment

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

So, there's actually no elegant way to get a list of running autoload singletons...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, I just can't find anything better so far.

@Ark2000
Copy link
Owner

Ark2000 commented Dec 13, 2023

The pr solved an issue also, #154.

It seems that you are already well acquainted with the code base. I'm excited to see more of your contributions in the future!

@Ark2000 Ark2000 self-assigned this Dec 13, 2023
@Ark2000 Ark2000 linked an issue Dec 13, 2023 that may be closed by this pull request
@worron worron requested a review from Ark2000 December 13, 2023 15:41
@worron
Copy link
Collaborator Author

worron commented Dec 13, 2023

Fixes done.

@Ark2000 Ark2000 merged commit 0f6c019 into Ark2000:master Dec 13, 2023
@worron worron deleted the variable_tracker_rework branch December 14, 2023 19:59
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.

Remove autoload will cause crash in dev console
2 participants