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

Refactor Process Mode #46191

Merged
merged 1 commit into from
Feb 19, 2021
Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Feb 18, 2021

Implements godotengine/godot-proposals#1835 (comment) , extending the pause functionality to a more generic process mode. This allows disabling of nodes, as well as their children.

Keep in mind that disabling nodes only affects the logic, and does not hide them. This goes separate to the visual part.

  • PauseMode is now ProcessMode, containing the following states:
     PROCESS_MODE_INHERIT, // same as parent node
     PROCESS_MODE_PAUSABLE, // process only if not paused
     PROCESS_MODE_WHEN_PAUSED, // process only if paused
     PROCESS_MODE_ALWAYS, // process always
     PROCESS_MODE_DISABLED, // never process
  • NOTIFICATION_PAUSED and NOTIFICATION_UNPAUSED are received effectively when the node is paused and unpaused (not any longer when pause mode is set in SceneTree).
  • Renamed some nodes that used ProcessMode/process_mode to specify a callback type to ProcessCallback to avoid clashes.

Obligatory Screenshot:

image

@reduz reduz requested review from a team as code owners February 18, 2021 20:22
@Calinou Calinou added this to the 4.0 milestone Feb 18, 2021
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Missing the renaming of SceneTreeTimer's is_pause_mode_process, since it's now called process mode, not pause mode.

Missing documentation. EDIT: I can add this though, I already have similar documentation written for #39606.

PROCESS_MODE_NORMAL, // process only if not paused
PROCESS_MODE_PAUSE_ONLY, // process only if paused
PROCESS_MODE_ALWAYS, // process always
PROCESS_MODE_DISABLED, // never process
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this one be called PROCESS_MODE_NEVER, if it's the opposite of ALWAYS?

Choose a reason for hiding this comment

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

Maybe because Never sounds too irreversible?
Disabled would suggest the possibility to (re-)activate it when requested?

Copy link
Member

Choose a reason for hiding this comment

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

If DISABLED is kept, then ALWAYS should be changed to ENABLED for consistency.

Copy link
Member Author

@reduz reduz Feb 18, 2021

Choose a reason for hiding this comment

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

I think current naming is more intuitive.
edit: changed it

Implements godotengine/godot-proposals#1835 (comment)

* PauseMode is now ProcessMode, containing the following states:
	```
	PROCESS_MODE_INHERIT, // same as parent node
	PROCESS_MODE_NORMAL, // process only if not paused
	PROCESS_MODE_PAUSE_ONLY, // process only if paused
	PROCESS_MODE_ALWAYS, // process always
	PROCESS_MODE_DISABLED, // never process
	```
* NOTIFICATION_PAUSED and NOTIFICATION_UNPAUSED are received effectively when the node is paused and unpaused (not any longer when pause mode is set in SceneTree).
* Renamed some nodes that used ProcessMode/process_mode to specify a callback type to ProcessCallback to avoid clashes.
Comment on lines +1764 to +1767
if (p.name == "script") {
category_vbox = nullptr; // script should go into its own category
}

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look like related to this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

It fixes a bug where the script appears above the process mode property, so I fixed it here.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I'm not convinced that this is 100% ideal, but it's not confusing and it's much better than the current system, so I will give this a ✔️

I can add documentation and do the SceneTreeTimer rename I mentioned, I'll rebase #39606 on top of this PR.

@akien-mga akien-mga merged commit 04cb7e6 into godotengine:master Feb 19, 2021
@akien-mga
Copy link
Member

Thanks!

Valla-Chan added a commit to Valla-Chan/Godot-3.5.1-Ghodost that referenced this pull request Nov 2, 2023
Valla-Chan added a commit to Valla-Chan/Godot-3.5.1-Ghodost that referenced this pull request Nov 2, 2023
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.

7 participants