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

Optimized changeDetector.gd, some static typing #4

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ImmortalOctogen
Copy link

  • changeDetector.gd now uses a thread, significantly increasing fps (at least on my laptop).
  • load() replaced with preload(), for static file paths.
  • other micro optimization, check comments

This is my first time contributing to other projects, and therefore, having poor experience working with git.
I started to write weeks ago, look for downgrades!

@Wolfyxon
Copy link
Owner

Wolfyxon commented Feb 23, 2025

There are a few issues, but overall you did a good job.
I'll make a review soon and fix some bugs on my side before merging.

@Wolfyxon
Copy link
Owner

Please make the changes

@ImmortalOctogen
Copy link
Author

It's okay when forked rep is 2 commits behind original?

@Wolfyxon
Copy link
Owner

Wolfyxon commented Feb 25, 2025

Usually ok since the PR can be merged anyway (unless there are conflicts, but in this case there are not), but I recommend you to always sync so you can work with the current state of the original code and when I'm testing it, it has my latest changes.

Basically it makes everything easier, but it's usually still fine if you keep the fork behind.

@Wolfyxon
Copy link
Owner

There's a big issue, get_property_dict() can't be called on the separate thread.
Idk how to fix that yet.

if not main: return

var root = EditorInterface.get_edited_scene_root()

var root := EditorInterface.call_deferred("get_edited_scene_root")
Copy link
Owner

Choose a reason for hiding this comment

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

call_deferred() doesn't return the called function's result.
Here's how it can be fixed:

func _ready() -> void:
	var t: Thread = Thread.new()
	refrate.wait_time = 0.1

	var root = EditorInterface.get_edited_scene_root() # Get scene root in the main thread

	refrate.timeout.connect(func():
		if t.start(cycle.bind(root)) == ERR_CANT_CREATE: # Pass `root` to the thread
			print_debug(error_string(ERR_CANT_CREATE))
			return
		while true:
			if not t.is_alive():
				t.wait_to_finish()
				break
	)

	add_child(refrate)
	refrate.start()
func cycle(root: Node) -> void:
	if not main: return
	if not root: return

        ....

@Wolfyxon
Copy link
Owner

Wolfyxon commented Feb 25, 2025

I can't get the functions to properly talk to each on the separate threads.
You should split this PR to include your other optimizations and we'll figure out multithreading later.

And next time I recommend you commit more frequently so splitting a PR would be as simple as going back to an older commit and creating a branch off of it then submitting a PR.

@ImmortalOctogen
Copy link
Author

I can't get the functions to properly talk to each on the separate threads. You should split this PR to include your other optimizations and we'll figure out multithreading later.

create other PR for multithreading issues, and leave this one for other optimizations?

@Wolfyxon
Copy link
Owner

Wolfyxon commented Feb 25, 2025

Yeah, remove the multithreading changes for this PR.
Then you can later add them to a next PR or make a branch from an older commit and add submit a separate PR.

Also what version of Godot are you using and did you test if the multithreading works for you?

Btw, I wrote some development tips in the CONTRIBUTING.md file.

@ImmortalOctogen
Copy link
Author

ImmortalOctogen commented Feb 26, 2025

It doesn't work for me either, I probably hastened with writing code. Godot version 4.3.stable

Btw, I wrote some development tips in the CONTRIBUTING.md file.

uzful :D

@Wolfyxon
Copy link
Owner

It doesn't work for me either, I probably hastened with writing code.

Make sure to test it next time

Godot version 4.3.stable

Latest, good

@ImmortalOctogen
Copy link
Author

ImmortalOctogen commented Feb 26, 2025

On enabling plugin, this shows up:
res://addons/GodotTogether/src/scripts/changeDetector.gd:48 - Invalid access to property or key 'surface_material_override/0' on a base object of type 'MeshInstance3D'.
AnimationPlayer has no current animation.

tf this come from? Probably from my other project, where I testing plugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants