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

Fix not being able to set Node process priority in certain cases #82358

Conversation

jsjtxietian
Copy link
Contributor

@jsjtxietian jsjtxietian commented Sep 26, 2023

Should fix #82339

I guess we can hide the process priority UI or just set the value without touching the thread_group part when _is_any_processing is false. This pr goes with the second option.

process_priority is only used in ComparatorWithPriority now so I guess this commit is safe to some extent. I would appreciate any feedback or alternative suggestions.

@jsjtxietian jsjtxietian requested a review from a team as a code owner September 26, 2023 10:03
@RandomShaper RandomShaper added this to the 4.2 milestone Sep 26, 2023
@RandomShaper RandomShaper added bug topic:core cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Sep 26, 2023
@RandomShaper
Copy link
Member

Looks good. The key thing is that items affecting node thread groups (namely, processing on/off and priority) can't be changed if the node is already in one group (i.e., processing is on). The current code was mistakenly preventing a legal change.

That said, for consistency I'd suggest the same pattern as in set_physics_process(); such as:

	if (_is_any_processing()) {
		_remove_from_process_thread_group();
	}

	data.process_priority = p_priority;

	if (_is_any_processing()) {
		_add_to_process_thread_group();
	}

@jsjtxietian jsjtxietian force-pushed the fix-can-not-set-process-priority-of-node-in-editor branch from 64641cc to e36117f Compare September 26, 2023 11:29
@akien-mga akien-mga merged commit 008b08b into godotengine:master Sep 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@jsjtxietian jsjtxietian deleted the fix-can-not-set-process-priority-of-node-in-editor branch September 26, 2023 12:04
@akien-mga akien-mga changed the title Fix can not set process priority of node in certain cases Fix not being able to set Node process priority in certain cases Oct 3, 2023
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

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.

Process priority unsettable in inspector
4 participants