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

Add session-id CLI Argument to Allow for Debugger Session Identification #64913

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

Conversation

Eoin-ONeill-Yokai
Copy link
Contributor

As discussed in godotengine/godot-proposals#3357 , I have added the ability for Godot to be passed a session identifier which is then used by the debugger whenever there are multiple debugger sessions launched (Debug > Multiple Instances configuration larger than 1.)

The Engine class has been modified by adding two methods: set_session_id and get_session_id; For each one I've added the appropriate documentation for their purpose and utility.

While using Godot's multiple-instance debugger, users can call Engine::get_session_id to branch logic for their needs. For example, the session id can be used to tell one instance to host a server while the others establish a peer connection. This is a nice quality of life feature for developers of multiplayer games where they might want to start a server and multiple clients simultaneously. Or you could use session ids to branch between two different gameplay configurations such as character walk speeds or jump heights. This is a nice way to compare and contrast two drastically different gameplay scenarios for game testing.

Here's a simplified idea on how this is supposed to be used within the editor.

# ...

enum NetworkRole {
	SERVER,
	CLIENT
}

func _enter_tree():
	if "--server" in OS.get_cmdline_args() or Engine.get_session_id() == 1:
		print("Server Start")
		start_network(NetworkRole.SERVER)
	else:
		print("Client Start")
		start_network(NetworkRole.CLIENT, "127.0.0.1")

# ...

From here, assuming multiple instances have been enabled by the user, the debugger window will open two or more windows with one instance hosting a server while the others connect as peers. This is essential for quickly testing networked multiplayer games.

Terminology Note

This patch refers to each Godot instance launched by the debugger as a "session", which is what they're called in the tabs above the debugger. However, in the Debug drop-down menu, these are referred as "instances". Since get_instance_id is already used extensively in the engine code, using the word session here seems appropriate. It might be worth considering changing the terminology in the debug menu to be consistent with the debugger tab at the bottom (i.e. Run Multiple Sessions / Run X Sessions.)

@Eoin-ONeill-Yokai Eoin-ONeill-Yokai requested review from a team as code owners August 26, 2022 05:40
@Eoin-ONeill-Yokai Eoin-ONeill-Yokai force-pushed the session_id branch 3 times, most recently from 3499e5a to ed0dd6d Compare August 26, 2022 07:48
@Calinou Calinou added this to the 4.0 milestone Aug 26, 2022
@Eoin-ONeill-Yokai Eoin-ONeill-Yokai force-pushed the session_id branch 3 times, most recently from 07939e9 to dd11da7 Compare August 29, 2022 18:38
This is used in the editor when you have multiple debug sessions and you
want to know which session is which. For online multiplayer development,
this is important since you can use these identities to determine
whether an instance should start as a server or a client. Additionally,
you could use this session id simply to cross compare two different game
sessions.
`get_session_id` and `set_session_id` have been added with proper
documentation.
Now `push_front` calls are tracked and accounted for in indexing when
each debugging instance is started.
@Eoin-ONeill-Yokai
Copy link
Contributor Author

I've rebased the commits to fix a merge conflict.

OS::ProcessID pid = 0;

if (instances_count > 1 && session_id_position >= 0) {
args[session_id_position + front_pushes] = String::num_int64(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that it's more simpler to add --session-id here.

@@ -1650,6 +1658,8 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph
GLOBAL_DEF("debug/settings/stdout/print_gpu_profile", false);
GLOBAL_DEF("debug/settings/stdout/verbose_stdout", false);

Engine::get_singleton()->set_session_id(init_session_id);
Copy link
Contributor

@kdada kdada Oct 23, 2022

Choose a reason for hiding this comment

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

The session ID (or index) may be not equal to the session index of RemoteDebugger because of the execution order of connections.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 10, 2023
@YuriSizov YuriSizov requested a review from a team February 10, 2023 15:40
@Eoin-ONeill-Yokai
Copy link
Contributor Author

@kdada does #68054 make this PR obsolete or are these two PRs slightly different from one another?

I still think that some command line options for session id might not be a terrible idea for testing outside of the editor, but perhaps that can be added on top of your PR?

Otherwise, I can fix up and rebase.

@IDreamInCode

This comment was marked as off-topic.

@Calinou
Copy link
Member

Calinou commented Apr 28, 2024

@IDreamInCode Please don't bump issues without contributing significant new information. Use the 👍 reaction button on the first post instead.


Regarding the PR itself, I wonder if the CLI argument should also send the total amount of instances, so you know how many instances were created even on the 1st instance. This may be useful if you want to apply different (faster) graphics settings when running more than 2 instances, and have these settings apply to all instances.

To avoid adding yet another CLI argument, the --session-id argument value could be formatted as follows: current/total, e.g. --session-id 2/4.

This isn't critical so it can be left to a future PR, but I thought it'd be worth mentioning.

@IDreamInCode
Copy link

IDreamInCode commented Apr 28, 2024 via email

@AThousandShips AThousandShips changed the title Added session-id CLI Argument to Allow for Debugger Session Identification Add session-id CLI Argument to Allow for Debugger Session Identification Apr 28, 2024
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.

5 participants